[PATCH-v4 0/3] mfd: 88pm800: Add Device tree support

[PATCH-v4 0/3] mfd: 88pm800: Add Device tree support

[Vaibhav Hiremath]

This patch-series adds support for Device tree to 88PM800 mfd driver.
It also sets default configuration of irq clear method if board file
doesn't exist.

Testing::
 - Boot tested on PXA1928 based platform.
 - probe of mfd, rtc and regulator function passing successfully.
 - Basic read operations on registers
 - irq clear configuration

V3 => V4
=======
Link to V3: https://lkml.org/lkml/2015/6/24/143

   - Removed generic error msg when returning -ENOMEM
   - irq clear method is 88PM800 feature, which is not dependent on board or
     doesn't require any wiring changes, so DT is not the way.
     Hardcoded to "irq clear on write" if board file doesn't exist.
   - Updated binding patch (PATCH 3/3) to remove irq-clr-on-wr entry.
   - Since PATCH 3/3 changed from original, removed Rob's Acked-by.

V2 => V3
=======
Link to V2: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg914299.html

   - Replaced deprecated "regulator-compatible" property with "regulator-name".
   - Added Rob's Acked-by  to [PATCH 3/3]

V1 => V2
=======
Link to V1: http://lkml.iu.edu/hypermail/linux/kernel/1505.3/04386.html

  - Split binding changes from original commit
  - Updated binding info as per Rob's suggestion
  - Dropped PATCH 4/4, as discussed during review
  - Dropped PATCH 3/4, as it is independent RTC code change,
    so will submit it separately to ease merging.
  - Fixed all other minor comments

Attempt has been made to push some of the patches to the list sometime
back in 2013.

Link to previous patch submission:
        https://lkml.org/lkml/2013/8/14/86

TODO:
=====
   - init config for 88PM860 device
   - Rgulator driver changes to add support for 88PM860 device


Vaibhav Hiremath (3):
  mfd: 88pm800: Add device tree support
  mfd: 88pm800: Set default interrupt clear method
  mfd: devicetree: bindings: Add new 88pm800 mfd binding

 Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
 drivers/mfd/88pm800.c                             | 38 +++++++++++++---
 include/linux/mfd/88pm80x.h                       |  6 ++-
 3 files changed, 91 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt

-- 
1.9.1

[PATCH-v4 0/3] mfd: 88pm800: Add Device tree support

[Vaibhav Hiremath]

This patch-series adds support for Device tree to 88PM800 mfd driver.
It also sets default configuration of irq clear method if board file
doesn't exist.

Testing::
 - Boot tested on PXA1928 based platform.
 - probe of mfd, rtc and regulator function passing successfully.
 - Basic read operations on registers
 - irq clear configuration

V3 => V4
=======
Link to V3: https://lkml.org/lkml/2015/6/24/143

   - Removed generic error msg when returning -ENOMEM
   - irq clear method is 88PM800 feature, which is not dependent on board or
     doesn't require any wiring changes, so DT is not the way.
     Hardcoded to "irq clear on write" if board file doesn't exist.
   - Updated binding patch (PATCH 3/3) to remove irq-clr-on-wr entry.
   - Since PATCH 3/3 changed from original, removed Rob's Acked-by.

V2 => V3
=======
Link to V2: https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg914299.html

   - Replaced deprecated "regulator-compatible" property with "regulator-name".
   - Added Rob's Acked-by  to [PATCH 3/3]

V1 => V2
=======
Link to V1: http://lkml.iu.edu/hypermail/linux/kernel/1505.3/04386.html

  - Split binding changes from original commit
  - Updated binding info as per Rob's suggestion
  - Dropped PATCH 4/4, as discussed during review
  - Dropped PATCH 3/4, as it is independent RTC code change,
    so will submit it separately to ease merging.
  - Fixed all other minor comments

Attempt has been made to push some of the patches to the list sometime
back in 2013.

Link to previous patch submission:
        https://lkml.org/lkml/2013/8/14/86

TODO:
=====
   - init config for 88PM860 device
   - Rgulator driver changes to add support for 88PM860 device


Vaibhav Hiremath (3):
  mfd: 88pm800: Add device tree support
  mfd: 88pm800: Set default interrupt clear method
  mfd: devicetree: bindings: Add new 88pm800 mfd binding

 Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
 drivers/mfd/88pm800.c                             | 38 +++++++++++++---
 include/linux/mfd/88pm80x.h                       |  6 ++-
 3 files changed, 91 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 0/3] mfd: 88pm800: Add Device tree support

[Vaibhav Hiremath]

This patch-series adds support for Device tree to 88PM800 mfd driver.
It also sets default configuration of irq clear method if board file
doesn't exist.

Testing::
 - Boot tested on PXA1928 based platform.
 - probe of mfd, rtc and regulator function passing successfully.
 - Basic read operations on registers
 - irq clear configuration

V3 => V4
=======
Link to V3: https://lkml.org/lkml/2015/6/24/143

   - Removed generic error msg when returning -ENOMEM
   - irq clear method is 88PM800 feature, which is not dependent on board or
     doesn't require any wiring changes, so DT is not the way.
     Hardcoded to "irq clear on write" if board file doesn't exist.
   - Updated binding patch (PATCH 3/3) to remove irq-clr-on-wr entry.
   - Since PATCH 3/3 changed from original, removed Rob's Acked-by.

V2 => V3
=======
Link to V2: https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg914299.html

   - Replaced deprecated "regulator-compatible" property with "regulator-name".
   - Added Rob's Acked-by  to [PATCH 3/3]

V1 => V2
=======
Link to V1: http://lkml.iu.edu/hypermail/linux/kernel/1505.3/04386.html

  - Split binding changes from original commit
  - Updated binding info as per Rob's suggestion
  - Dropped PATCH 4/4, as discussed during review
  - Dropped PATCH 3/4, as it is independent RTC code change,
    so will submit it separately to ease merging.
  - Fixed all other minor comments

Attempt has been made to push some of the patches to the list sometime
back in 2013.

Link to previous patch submission:
        https://lkml.org/lkml/2013/8/14/86

TODO:
=====
   - init config for 88PM860 device
   - Rgulator driver changes to add support for 88PM860 device


Vaibhav Hiremath (3):
  mfd: 88pm800: Add device tree support
  mfd: 88pm800: Set default interrupt clear method
  mfd: devicetree: bindings: Add new 88pm800 mfd binding

 Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
 drivers/mfd/88pm800.c                             | 38 +++++++++++++---
 include/linux/mfd/88pm80x.h                       |  6 ++-
 3 files changed, 91 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt

-- 
1.9.1

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

Add DT support to the 88pm800 driver, along with compatible
field for it's sub-devices (rtc, onkey and regulator)

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

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 841717a..40fd014 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -27,6 +27,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/88pm80x.h>
 #include <linux/slab.h>
+#include <linux/of_device.h>
 
 /* Interrupt Registers */
 #define PM800_INT_STATUS1		(0x05)
@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
 
+static const struct of_device_id pm80x_of_match_table[] = {
+	{ .compatible = "marvell,88pm800", },
+	{},
+};
+
 static struct resource rtc_resources[] = {
 	{
 	 .name = "88pm80x-rtc",
@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
 static struct mfd_cell rtc_devs[] = {
 	{
 	 .name = "88pm80x-rtc",
+	 .of_compatible = "marvell,88pm80x-rtc",
 	 .num_resources = ARRAY_SIZE(rtc_resources),
 	 .resources = &rtc_resources[0],
 	 .id = -1,
@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
 static const struct mfd_cell onkey_devs[] = {
 	{
 	 .name = "88pm80x-onkey",
+	 .of_compatible = "marvell,88pm80x-onkey",
 	 .num_resources = 1,
 	 .resources = &onkey_resources[0],
 	 .id = -1,
@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
 static const struct mfd_cell regulator_devs[] = {
 	{
 	 .name = "88pm80x-regulator",
+	 .of_compatible = "marvell,88pm80x-regulator",
 	 .id = -1,
 	},
 };
@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
 	int ret = 0;
 	struct pm80x_chip *chip;
 	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct device_node *np = client->dev.of_node;
 	struct pm80x_subchip *subchip;
 
+	if (!pdata && !np) {
+		dev_err(&client->dev,
+			"pm80x requires platform data or of_node\n");
+		return -EINVAL;
+	}
+
+	if (!pdata) {
+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+	}
+
 	ret = pm80x_init(client);
 	if (ret) {
 		dev_err(&client->dev, "pm800_init fail\n");
@@ -611,6 +633,7 @@ static struct i2c_driver pm800_driver = {
 		.name = "88PM800",
 		.owner = THIS_MODULE,
 		.pm = &pm80x_pm_ops,
+		.of_match_table	= pm80x_of_match_table,
 		},
 	.probe = pm800_probe,
 	.remove = pm800_remove,
-- 
1.9.1

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

Add DT support to the 88pm800 driver, along with compatible
field for it's sub-devices (rtc, onkey and regulator)

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

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 841717a..40fd014 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -27,6 +27,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/88pm80x.h>
 #include <linux/slab.h>
+#include <linux/of_device.h>
 
 /* Interrupt Registers */
 #define PM800_INT_STATUS1		(0x05)
@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
 
+static const struct of_device_id pm80x_of_match_table[] = {
+	{ .compatible = "marvell,88pm800", },
+	{},
+};
+
 static struct resource rtc_resources[] = {
 	{
 	 .name = "88pm80x-rtc",
@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
 static struct mfd_cell rtc_devs[] = {
 	{
 	 .name = "88pm80x-rtc",
+	 .of_compatible = "marvell,88pm80x-rtc",
 	 .num_resources = ARRAY_SIZE(rtc_resources),
 	 .resources = &rtc_resources[0],
 	 .id = -1,
@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
 static const struct mfd_cell onkey_devs[] = {
 	{
 	 .name = "88pm80x-onkey",
+	 .of_compatible = "marvell,88pm80x-onkey",
 	 .num_resources = 1,
 	 .resources = &onkey_resources[0],
 	 .id = -1,
@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
 static const struct mfd_cell regulator_devs[] = {
 	{
 	 .name = "88pm80x-regulator",
+	 .of_compatible = "marvell,88pm80x-regulator",
 	 .id = -1,
 	},
 };
@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
 	int ret = 0;
 	struct pm80x_chip *chip;
 	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct device_node *np = client->dev.of_node;
 	struct pm80x_subchip *subchip;
 
+	if (!pdata && !np) {
+		dev_err(&client->dev,
+			"pm80x requires platform data or of_node\n");
+		return -EINVAL;
+	}
+
+	if (!pdata) {
+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+	}
+
 	ret = pm80x_init(client);
 	if (ret) {
 		dev_err(&client->dev, "pm800_init fail\n");
@@ -611,6 +633,7 @@ static struct i2c_driver pm800_driver = {
 		.name = "88PM800",
 		.owner = THIS_MODULE,
 		.pm = &pm80x_pm_ops,
+		.of_match_table	= pm80x_of_match_table,
 		},
 	.probe = pm800_probe,
 	.remove = pm800_remove,
-- 
1.9.1

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
(page 0) controls the method of clearing interrupt
status of 88pm800 family of devices;

  0: clear on read
  1: clear on write

If pdata is not coming from board file, then set the
default irq clear method to "irq clear on write"

Also, as suggested by "Lee Jones" renaming variable field
to appropriate name.

Signed-off-by: Zhao Ye <zhaoy@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mfd/88pm800.c       | 15 ++++++++++-----
 include/linux/mfd/88pm80x.h |  6 ++++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 40fd014..e0cd7ad 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 {
 	struct regmap *map = chip->regmap;
 	unsigned long flags = IRQF_ONESHOT;
-	int data, mask, ret = -EINVAL;
+	int irq_clr_mode, mask, ret = -EINVAL;
 
 	if (!map || !chip->irq) {
 		dev_err(chip->dev, "incorrect parameters\n");
@@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 	}
 
 	/*
-	 * irq_mode defines the way of clearing interrupt. it's read-clear by
-	 * default.
+	 * irq_clr_on_wr defines the way of clearing interrupt by
+	 * read/write(0/1).  It's read-clear by default.
 	 */
 	mask =
 	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
 	    PM800_WAKEUP2_INT_MASK;
 
-	data = PM800_WAKEUP2_INT_CLEAR;
-	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
+	irq_clr_mode = (chip->irq_clr_on_wr) ?
+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
 
 	if (ret < 0)
 		goto out;
@@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
 	}
 
 	chip->regmap_irq_chip = &pm800_irq_chip;
+	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
 
 	ret = device_irq_init_800(chip);
 	if (ret < 0) {
@@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
 		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
 		if (!pdata)
 			return -ENOMEM;
+
+		/* by default, set irq clear method on write */
+		pdata->irq_clr_on_wr = true;
 	}
 
 	ret = pm80x_init(client);
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 97cb283..94b3dcd 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)
@@ -300,7 +302,7 @@ struct pm80x_chip {
 	struct regmap_irq_chip_data *irq_data;
 	int type;
 	int irq;
-	int irq_mode;
+	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
 	unsigned long wu_flag;
 	spinlock_t lock;
 };
@@ -315,7 +317,7 @@ struct pm80x_platform_data {
 	 */
 	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
 	unsigned int num_regulators;
-	int irq_mode;		/* Clear interrupt by read/write(0/1) */
+	int irq_clr_on_wr;		/* Clear interrupt by read/write(0/1) */
 	int batt_det;		/* enable/disable */
 	int (*plat_config)(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata);
-- 
1.9.1

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
(page 0) controls the method of clearing interrupt
status of 88pm800 family of devices;

  0: clear on read
  1: clear on write

If pdata is not coming from board file, then set the
default irq clear method to "irq clear on write"

Also, as suggested by "Lee Jones" renaming variable field
to appropriate name.

Signed-off-by: Zhao Ye <zhaoy@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mfd/88pm800.c       | 15 ++++++++++-----
 include/linux/mfd/88pm80x.h |  6 ++++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 40fd014..e0cd7ad 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 {
 	struct regmap *map = chip->regmap;
 	unsigned long flags = IRQF_ONESHOT;
-	int data, mask, ret = -EINVAL;
+	int irq_clr_mode, mask, ret = -EINVAL;
 
 	if (!map || !chip->irq) {
 		dev_err(chip->dev, "incorrect parameters\n");
@@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 	}
 
 	/*
-	 * irq_mode defines the way of clearing interrupt. it's read-clear by
-	 * default.
+	 * irq_clr_on_wr defines the way of clearing interrupt by
+	 * read/write(0/1).  It's read-clear by default.
 	 */
 	mask =
 	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
 	    PM800_WAKEUP2_INT_MASK;
 
-	data = PM800_WAKEUP2_INT_CLEAR;
-	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
+	irq_clr_mode = (chip->irq_clr_on_wr) ?
+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
 
 	if (ret < 0)
 		goto out;
@@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
 	}
 
 	chip->regmap_irq_chip = &pm800_irq_chip;
+	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
 
 	ret = device_irq_init_800(chip);
 	if (ret < 0) {
@@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
 		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
 		if (!pdata)
 			return -ENOMEM;
+
+		/* by default, set irq clear method on write */
+		pdata->irq_clr_on_wr = true;
 	}
 
 	ret = pm80x_init(client);
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 97cb283..94b3dcd 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)
@@ -300,7 +302,7 @@ struct pm80x_chip {
 	struct regmap_irq_chip_data *irq_data;
 	int type;
 	int irq;
-	int irq_mode;
+	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
 	unsigned long wu_flag;
 	spinlock_t lock;
 };
@@ -315,7 +317,7 @@ struct pm80x_platform_data {
 	 */
 	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
 	unsigned int num_regulators;
-	int irq_mode;		/* Clear interrupt by read/write(0/1) */
+	int irq_clr_on_wr;		/* Clear interrupt by read/write(0/1) */
 	int batt_det;		/* enable/disable */
 	int (*plat_config)(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata);
-- 
1.9.1

[PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

With addition of DT support to 88pm800 mfd driver, this patch
adds new DT binding documentation along with respective properties.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
 1 file changed, 54 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..f56b751
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -0,0 +1,54 @@
+* Marvell 88PM8xx Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
+- reg : the I2C slave address for the 88pm8xx chip
+- interrupts : IRQ line for the 88pm8xx chip
+- interrupt-controller: describes the 88pm8xx as an interrupt controller
+- #interrupt-cells : should be 1.
+		- The cell is the 88pm8xx local IRQ number
+
+88pm8xx family of devices consists of varied group of sub-devices:
+
+Device		 	Supply Names	 Description
+------		 	------------	 -----------
+88pm80x-onkey		:		: On key
+88pm80x-rtc		:		: RTC
+88pm80x-regulator	:		: Regulators
+
+Note: More device list will follow
+
+Example:
+
+	pmic: 88pm800@30 {
+		compatible = "marvell,88pm800";
+		reg = <0x30>;
+		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-parent = <&gic>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		regulators {
+			compatible = "marvell,88pm80x-regulator";
+
+			buck1a: BUCK1A {
+				regulator-name = "BUCK1A";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo1: LDO1 {
+				regulator-name = "LDO1";
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+
+		rtc {
+			compatible = "marvell,88pm80x-rtc";
+		};
+	};
-- 
1.9.1

[PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

With addition of DT support to 88pm800 mfd driver, this patch
adds new DT binding documentation along with respective properties.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
 1 file changed, 54 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..f56b751
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -0,0 +1,54 @@
+* Marvell 88PM8xx Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
+- reg : the I2C slave address for the 88pm8xx chip
+- interrupts : IRQ line for the 88pm8xx chip
+- interrupt-controller: describes the 88pm8xx as an interrupt controller
+- #interrupt-cells : should be 1.
+		- The cell is the 88pm8xx local IRQ number
+
+88pm8xx family of devices consists of varied group of sub-devices:
+
+Device		 	Supply Names	 Description
+------		 	------------	 -----------
+88pm80x-onkey		:		: On key
+88pm80x-rtc		:		: RTC
+88pm80x-regulator	:		: Regulators
+
+Note: More device list will follow
+
+Example:
+
+	pmic: 88pm800@30 {
+		compatible = "marvell,88pm800";
+		reg = <0x30>;
+		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-parent = <&gic>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		regulators {
+			compatible = "marvell,88pm80x-regulator";
+
+			buck1a: BUCK1A {
+				regulator-name = "BUCK1A";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo1: LDO1 {
+				regulator-name = "LDO1";
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+
+		rtc {
+			compatible = "marvell,88pm80x-rtc";
+		};
+	};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

With addition of DT support to 88pm800 mfd driver, this patch
adds new DT binding documentation along with respective properties.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
 1 file changed, 54 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..f56b751
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -0,0 +1,54 @@
+* Marvell 88PM8xx Power Management IC
+
+Required parent device properties:
+- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
+- reg : the I2C slave address for the 88pm8xx chip
+- interrupts : IRQ line for the 88pm8xx chip
+- interrupt-controller: describes the 88pm8xx as an interrupt controller
+- #interrupt-cells : should be 1.
+		- The cell is the 88pm8xx local IRQ number
+
+88pm8xx family of devices consists of varied group of sub-devices:
+
+Device		 	Supply Names	 Description
+------		 	------------	 -----------
+88pm80x-onkey		:		: On key
+88pm80x-rtc		:		: RTC
+88pm80x-regulator	:		: Regulators
+
+Note: More device list will follow
+
+Example:
+
+	pmic: 88pm800 at 30 {
+		compatible = "marvell,88pm800";
+		reg = <0x30>;
+		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-parent = <&gic>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		regulators {
+			compatible = "marvell,88pm80x-regulator";
+
+			buck1a: BUCK1A {
+				regulator-name = "BUCK1A";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo1: LDO1 {
+				regulator-name = "LDO1";
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+
+		rtc {
+			compatible = "marvell,88pm80x-rtc";
+		};
+	};
-- 
1.9.1

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> Add DT support to the 88pm800 driver, along with compatible
> field for it's sub-devices (rtc, onkey and regulator)
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 841717a..40fd014 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -27,6 +27,7 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/88pm80x.h>
>  #include <linux/slab.h>
> +#include <linux/of_device.h>
>  
>  /* Interrupt Registers */
>  #define PM800_INT_STATUS1		(0x05)
> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>  
> +static const struct of_device_id pm80x_of_match_table[] = {
> +	{ .compatible = "marvell,88pm800", },
> +	{},
> +};
> +
>  static struct resource rtc_resources[] = {
>  	{
>  	 .name = "88pm80x-rtc",
> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>  static struct mfd_cell rtc_devs[] = {
>  	{
>  	 .name = "88pm80x-rtc",
> +	 .of_compatible = "marvell,88pm80x-rtc",
>  	 .num_resources = ARRAY_SIZE(rtc_resources),
>  	 .resources = &rtc_resources[0],
>  	 .id = -1,
> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>  static const struct mfd_cell onkey_devs[] = {
>  	{
>  	 .name = "88pm80x-onkey",
> +	 .of_compatible = "marvell,88pm80x-onkey",
>  	 .num_resources = 1,
>  	 .resources = &onkey_resources[0],
>  	 .id = -1,
> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>  static const struct mfd_cell regulator_devs[] = {
>  	{
>  	 .name = "88pm80x-regulator",
> +	 .of_compatible = "marvell,88pm80x-regulator",
>  	 .id = -1,
>  	},
>  };
> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>  	int ret = 0;
>  	struct pm80x_chip *chip;
>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct device_node *np = client->dev.of_node;
>  	struct pm80x_subchip *subchip;
>  
> +	if (!pdata && !np) {
> +		dev_err(&client->dev,
> +			"pm80x requires platform data or of_node\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pdata) {
> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +	}

Why have you allocated data for pdata, then done nothing with it?

>  	ret = pm80x_init(client);
>  	if (ret) {
>  		dev_err(&client->dev, "pm800_init fail\n");
> @@ -611,6 +633,7 @@ static struct i2c_driver pm800_driver = {
>  		.name = "88PM800",
>  		.owner = THIS_MODULE,
>  		.pm = &pm80x_pm_ops,
> +		.of_match_table	= pm80x_of_match_table,
>  		},
>  	.probe = pm800_probe,
>  	.remove = pm800_remove,

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

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> Add DT support to the 88pm800 driver, along with compatible
> field for it's sub-devices (rtc, onkey and regulator)
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 841717a..40fd014 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -27,6 +27,7 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/88pm80x.h>
>  #include <linux/slab.h>
> +#include <linux/of_device.h>
>  
>  /* Interrupt Registers */
>  #define PM800_INT_STATUS1		(0x05)
> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>  
> +static const struct of_device_id pm80x_of_match_table[] = {
> +	{ .compatible = "marvell,88pm800", },
> +	{},
> +};
> +
>  static struct resource rtc_resources[] = {
>  	{
>  	 .name = "88pm80x-rtc",
> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>  static struct mfd_cell rtc_devs[] = {
>  	{
>  	 .name = "88pm80x-rtc",
> +	 .of_compatible = "marvell,88pm80x-rtc",
>  	 .num_resources = ARRAY_SIZE(rtc_resources),
>  	 .resources = &rtc_resources[0],
>  	 .id = -1,
> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>  static const struct mfd_cell onkey_devs[] = {
>  	{
>  	 .name = "88pm80x-onkey",
> +	 .of_compatible = "marvell,88pm80x-onkey",
>  	 .num_resources = 1,
>  	 .resources = &onkey_resources[0],
>  	 .id = -1,
> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>  static const struct mfd_cell regulator_devs[] = {
>  	{
>  	 .name = "88pm80x-regulator",
> +	 .of_compatible = "marvell,88pm80x-regulator",
>  	 .id = -1,
>  	},
>  };
> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>  	int ret = 0;
>  	struct pm80x_chip *chip;
>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct device_node *np = client->dev.of_node;
>  	struct pm80x_subchip *subchip;
>  
> +	if (!pdata && !np) {
> +		dev_err(&client->dev,
> +			"pm80x requires platform data or of_node\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pdata) {
> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +	}

Why have you allocated data for pdata, then done nothing with it?

>  	ret = pm80x_init(client);
>  	if (ret) {
>  		dev_err(&client->dev, "pm800_init fail\n");
> @@ -611,6 +633,7 @@ static struct i2c_driver pm800_driver = {
>  		.name = "88PM800",
>  		.owner = THIS_MODULE,
>  		.pm = &pm80x_pm_ops,
> +		.of_match_table	= pm80x_of_match_table,
>  		},
>  	.probe = pm800_probe,
>  	.remove = pm800_remove,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> Add DT support to the 88pm800 driver, along with compatible
> field for it's sub-devices (rtc, onkey and regulator)
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 841717a..40fd014 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -27,6 +27,7 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/88pm80x.h>
>  #include <linux/slab.h>
> +#include <linux/of_device.h>
>  
>  /* Interrupt Registers */
>  #define PM800_INT_STATUS1		(0x05)
> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>  
> +static const struct of_device_id pm80x_of_match_table[] = {
> +	{ .compatible = "marvell,88pm800", },
> +	{},
> +};
> +
>  static struct resource rtc_resources[] = {
>  	{
>  	 .name = "88pm80x-rtc",
> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>  static struct mfd_cell rtc_devs[] = {
>  	{
>  	 .name = "88pm80x-rtc",
> +	 .of_compatible = "marvell,88pm80x-rtc",
>  	 .num_resources = ARRAY_SIZE(rtc_resources),
>  	 .resources = &rtc_resources[0],
>  	 .id = -1,
> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>  static const struct mfd_cell onkey_devs[] = {
>  	{
>  	 .name = "88pm80x-onkey",
> +	 .of_compatible = "marvell,88pm80x-onkey",
>  	 .num_resources = 1,
>  	 .resources = &onkey_resources[0],
>  	 .id = -1,
> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>  static const struct mfd_cell regulator_devs[] = {
>  	{
>  	 .name = "88pm80x-regulator",
> +	 .of_compatible = "marvell,88pm80x-regulator",
>  	 .id = -1,
>  	},
>  };
> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>  	int ret = 0;
>  	struct pm80x_chip *chip;
>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct device_node *np = client->dev.of_node;
>  	struct pm80x_subchip *subchip;
>  
> +	if (!pdata && !np) {
> +		dev_err(&client->dev,
> +			"pm80x requires platform data or of_node\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!pdata) {
> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +	}

Why have you allocated data for pdata, then done nothing with it?

>  	ret = pm80x_init(client);
>  	if (ret) {
>  		dev_err(&client->dev, "pm800_init fail\n");
> @@ -611,6 +633,7 @@ static struct i2c_driver pm800_driver = {
>  		.name = "88PM800",
>  		.owner = THIS_MODULE,
>  		.pm = &pm80x_pm_ops,
> +		.of_match_table	= pm80x_of_match_table,
>  		},
>  	.probe = pm800_probe,
>  	.remove = pm800_remove,

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

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> (page 0) controls the method of clearing interrupt
> status of 88pm800 family of devices;
> 
>   0: clear on read
>   1: clear on write
> 
> If pdata is not coming from board file, then set the
> default irq clear method to "irq clear on write"
> 
> Also, as suggested by "Lee Jones" renaming variable field
> to appropriate name.
> 
> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
>  include/linux/mfd/88pm80x.h |  6 ++++--
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 40fd014..e0cd7ad 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  {
>  	struct regmap *map = chip->regmap;
>  	unsigned long flags = IRQF_ONESHOT;
> -	int data, mask, ret = -EINVAL;
> +	int irq_clr_mode, mask, ret = -EINVAL;
>  
>  	if (!map || !chip->irq) {
>  		dev_err(chip->dev, "incorrect parameters\n");
> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  	}
>  
>  	/*
> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
> -	 * default.
> +	 * irq_clr_on_wr defines the way of clearing interrupt by
> +	 * read/write(0/1).  It's read-clear by default.
>  	 */
>  	mask =
>  	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>  	    PM800_WAKEUP2_INT_MASK;
>  
> -	data = PM800_WAKEUP2_INT_CLEAR;
> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> +	irq_clr_mode = (chip->irq_clr_on_wr) ?

Drop the brackets.

> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>  
>  	if (ret < 0)
>  		goto out;
> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  	}
>  
>  	chip->regmap_irq_chip = &pm800_irq_chip;
> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;

You have protection around pdata everywhere else in the file, I
suggest you supply some here too.

>  	ret = device_irq_init_800(chip);
>  	if (ret < 0) {
> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>  		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>  		if (!pdata)
>  			return -ENOMEM;
> +
> +		/* by default, set irq clear method on write */
> +		pdata->irq_clr_on_wr = true;

You can save yourself some memory here, by removing this seemingly
pointless allocation and do this above:

        chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;

>  	}
>  
>  	ret = pm80x_init(client);
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 97cb283..94b3dcd 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)

Use the BIT() macro.

>  #define PM800_POWER_UP_LOG	(0x10)
> @@ -300,7 +302,7 @@ struct pm80x_chip {
>  	struct regmap_irq_chip_data *irq_data;
>  	int type;
>  	int irq;
> -	int irq_mode;
> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/

Whitespace issue.

Shouldn't this be a bool?

Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
and call the variable irq_clear_method, or something.

Much clearer that way I think.

>  	unsigned long wu_flag;
>  	spinlock_t lock;
>  };
> @@ -315,7 +317,7 @@ struct pm80x_platform_data {
>  	 */
>  	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
>  	unsigned int num_regulators;
> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
> +	int irq_clr_on_wr;		/* Clear interrupt by read/write(0/1) */
>  	int batt_det;		/* enable/disable */
>  	int (*plat_config)(struct pm80x_chip *chip,
>  				struct pm80x_platform_data *pdata);

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

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> (page 0) controls the method of clearing interrupt
> status of 88pm800 family of devices;
> 
>   0: clear on read
>   1: clear on write
> 
> If pdata is not coming from board file, then set the
> default irq clear method to "irq clear on write"
> 
> Also, as suggested by "Lee Jones" renaming variable field
> to appropriate name.
> 
> Signed-off-by: Zhao Ye <zhaoy-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
>  include/linux/mfd/88pm80x.h |  6 ++++--
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 40fd014..e0cd7ad 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  {
>  	struct regmap *map = chip->regmap;
>  	unsigned long flags = IRQF_ONESHOT;
> -	int data, mask, ret = -EINVAL;
> +	int irq_clr_mode, mask, ret = -EINVAL;
>  
>  	if (!map || !chip->irq) {
>  		dev_err(chip->dev, "incorrect parameters\n");
> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  	}
>  
>  	/*
> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
> -	 * default.
> +	 * irq_clr_on_wr defines the way of clearing interrupt by
> +	 * read/write(0/1).  It's read-clear by default.
>  	 */
>  	mask =
>  	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>  	    PM800_WAKEUP2_INT_MASK;
>  
> -	data = PM800_WAKEUP2_INT_CLEAR;
> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> +	irq_clr_mode = (chip->irq_clr_on_wr) ?

Drop the brackets.

> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>  
>  	if (ret < 0)
>  		goto out;
> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  	}
>  
>  	chip->regmap_irq_chip = &pm800_irq_chip;
> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;

You have protection around pdata everywhere else in the file, I
suggest you supply some here too.

>  	ret = device_irq_init_800(chip);
>  	if (ret < 0) {
> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>  		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>  		if (!pdata)
>  			return -ENOMEM;
> +
> +		/* by default, set irq clear method on write */
> +		pdata->irq_clr_on_wr = true;

You can save yourself some memory here, by removing this seemingly
pointless allocation and do this above:

        chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;

>  	}
>  
>  	ret = pm80x_init(client);
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 97cb283..94b3dcd 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)

Use the BIT() macro.

>  #define PM800_POWER_UP_LOG	(0x10)
> @@ -300,7 +302,7 @@ struct pm80x_chip {
>  	struct regmap_irq_chip_data *irq_data;
>  	int type;
>  	int irq;
> -	int irq_mode;
> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/

Whitespace issue.

Shouldn't this be a bool?

Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
and call the variable irq_clear_method, or something.

Much clearer that way I think.

>  	unsigned long wu_flag;
>  	spinlock_t lock;
>  };
> @@ -315,7 +317,7 @@ struct pm80x_platform_data {
>  	 */
>  	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
>  	unsigned int num_regulators;
> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
> +	int irq_clr_on_wr;		/* Clear interrupt by read/write(0/1) */
>  	int batt_det;		/* enable/disable */
>  	int (*plat_config)(struct pm80x_chip *chip,
>  				struct pm80x_platform_data *pdata);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> (page 0) controls the method of clearing interrupt
> status of 88pm800 family of devices;
> 
>   0: clear on read
>   1: clear on write
> 
> If pdata is not coming from board file, then set the
> default irq clear method to "irq clear on write"
> 
> Also, as suggested by "Lee Jones" renaming variable field
> to appropriate name.
> 
> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
>  include/linux/mfd/88pm80x.h |  6 ++++--
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 40fd014..e0cd7ad 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  {
>  	struct regmap *map = chip->regmap;
>  	unsigned long flags = IRQF_ONESHOT;
> -	int data, mask, ret = -EINVAL;
> +	int irq_clr_mode, mask, ret = -EINVAL;
>  
>  	if (!map || !chip->irq) {
>  		dev_err(chip->dev, "incorrect parameters\n");
> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  	}
>  
>  	/*
> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
> -	 * default.
> +	 * irq_clr_on_wr defines the way of clearing interrupt by
> +	 * read/write(0/1).  It's read-clear by default.
>  	 */
>  	mask =
>  	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>  	    PM800_WAKEUP2_INT_MASK;
>  
> -	data = PM800_WAKEUP2_INT_CLEAR;
> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> +	irq_clr_mode = (chip->irq_clr_on_wr) ?

Drop the brackets.

> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>  
>  	if (ret < 0)
>  		goto out;
> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  	}
>  
>  	chip->regmap_irq_chip = &pm800_irq_chip;
> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;

You have protection around pdata everywhere else in the file, I
suggest you supply some here too.

>  	ret = device_irq_init_800(chip);
>  	if (ret < 0) {
> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>  		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>  		if (!pdata)
>  			return -ENOMEM;
> +
> +		/* by default, set irq clear method on write */
> +		pdata->irq_clr_on_wr = true;

You can save yourself some memory here, by removing this seemingly
pointless allocation and do this above:

        chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;

>  	}
>  
>  	ret = pm80x_init(client);
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 97cb283..94b3dcd 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)

Use the BIT() macro.

>  #define PM800_POWER_UP_LOG	(0x10)
> @@ -300,7 +302,7 @@ struct pm80x_chip {
>  	struct regmap_irq_chip_data *irq_data;
>  	int type;
>  	int irq;
> -	int irq_mode;
> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/

Whitespace issue.

Shouldn't this be a bool?

Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
and call the variable irq_clear_method, or something.

Much clearer that way I think.

>  	unsigned long wu_flag;
>  	spinlock_t lock;
>  };
> @@ -315,7 +317,7 @@ struct pm80x_platform_data {
>  	 */
>  	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
>  	unsigned int num_regulators;
> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
> +	int irq_clr_on_wr;		/* Clear interrupt by read/write(0/1) */
>  	int batt_det;		/* enable/disable */
>  	int (*plat_config)(struct pm80x_chip *chip,
>  				struct pm80x_platform_data *pdata);

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

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>  1 file changed, 54 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..f56b751
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,54 @@
> +* Marvell 88PM8xx Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm8xx chip
> +- interrupts : IRQ line for the 88pm8xx chip
> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm8xx local IRQ number

Might just be a personal thing, but I would line up the ':', it's much
easier on the eyes that way.

> +88pm8xx family of devices consists of varied group of sub-devices:
> +
> +Device		 	Supply Names	 Description
> +------		 	------------	 -----------
> +88pm80x-onkey		:		: On key
> +88pm80x-rtc		:		: RTC
> +88pm80x-regulator	:		: Regulators
> +
> +Note: More device list will follow

No real need for this.

> +Example:
> +
> +	pmic: 88pm800@30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		regulators {
> +			compatible = "marvell,88pm80x-regulator";
> +
> +			buck1a: BUCK1A {
> +				regulator-name = "BUCK1A";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1: LDO1 {
> +				regulator-name = "LDO1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +
> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};

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

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>  1 file changed, 54 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..f56b751
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,54 @@
> +* Marvell 88PM8xx Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm8xx chip
> +- interrupts : IRQ line for the 88pm8xx chip
> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm8xx local IRQ number

Might just be a personal thing, but I would line up the ':', it's much
easier on the eyes that way.

> +88pm8xx family of devices consists of varied group of sub-devices:
> +
> +Device		 	Supply Names	 Description
> +------		 	------------	 -----------
> +88pm80x-onkey		:		: On key
> +88pm80x-rtc		:		: RTC
> +88pm80x-regulator	:		: Regulators
> +
> +Note: More device list will follow

No real need for this.

> +Example:
> +
> +	pmic: 88pm800@30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		regulators {
> +			compatible = "marvell,88pm80x-regulator";
> +
> +			buck1a: BUCK1A {
> +				regulator-name = "BUCK1A";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1: LDO1 {
> +				regulator-name = "LDO1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +
> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:

> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>  1 file changed, 54 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..f56b751
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,54 @@
> +* Marvell 88PM8xx Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm8xx chip
> +- interrupts : IRQ line for the 88pm8xx chip
> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm8xx local IRQ number

Might just be a personal thing, but I would line up the ':', it's much
easier on the eyes that way.

> +88pm8xx family of devices consists of varied group of sub-devices:
> +
> +Device		 	Supply Names	 Description
> +------		 	------------	 -----------
> +88pm80x-onkey		:		: On key
> +88pm80x-rtc		:		: RTC
> +88pm80x-regulator	:		: Regulators
> +
> +Note: More device list will follow

No real need for this.

> +Example:
> +
> +	pmic: 88pm800 at 30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		regulators {
> +			compatible = "marvell,88pm80x-regulator";
> +
> +			buck1a: BUCK1A {
> +				regulator-name = "BUCK1A";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1: LDO1 {
> +				regulator-name = "LDO1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +
> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};

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

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> Add DT support to the 88pm800 driver, along with compatible
>> field for it's sub-devices (rtc, onkey and regulator)
>>
>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 841717a..40fd014 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -27,6 +27,7 @@
>>   #include <linux/mfd/core.h>
>>   #include <linux/mfd/88pm80x.h>
>>   #include <linux/slab.h>
>> +#include <linux/of_device.h>
>>
>>   /* Interrupt Registers */
>>   #define PM800_INT_STATUS1		(0x05)
>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>>   };
>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>
>> +static const struct of_device_id pm80x_of_match_table[] = {
>> +	{ .compatible = "marvell,88pm800", },
>> +	{},
>> +};
>> +
>>   static struct resource rtc_resources[] = {
>>   	{
>>   	 .name = "88pm80x-rtc",
>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>   static struct mfd_cell rtc_devs[] = {
>>   	{
>>   	 .name = "88pm80x-rtc",
>> +	 .of_compatible = "marvell,88pm80x-rtc",
>>   	 .num_resources = ARRAY_SIZE(rtc_resources),
>>   	 .resources = &rtc_resources[0],
>>   	 .id = -1,
>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>   static const struct mfd_cell onkey_devs[] = {
>>   	{
>>   	 .name = "88pm80x-onkey",
>> +	 .of_compatible = "marvell,88pm80x-onkey",
>>   	 .num_resources = 1,
>>   	 .resources = &onkey_resources[0],
>>   	 .id = -1,
>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>   static const struct mfd_cell regulator_devs[] = {
>>   	{
>>   	 .name = "88pm80x-regulator",
>> +	 .of_compatible = "marvell,88pm80x-regulator",
>>   	 .id = -1,
>>   	},
>>   };
>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>>   	int ret = 0;
>>   	struct pm80x_chip *chip;
>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>> +	struct device_node *np = client->dev.of_node;
>>   	struct pm80x_subchip *subchip;
>>
>> +	if (!pdata && !np) {
>> +		dev_err(&client->dev,
>> +			"pm80x requires platform data or of_node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!pdata) {
>> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> +		if (!pdata)
>> +			return -ENOMEM;
>> +	}
>
> Why have you allocated data for pdata, then done nothing with it?
>

Not in this patch, but subsequent patches would use it.

Thanks,
Vaibhav

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> Add DT support to the 88pm800 driver, along with compatible
>> field for it's sub-devices (rtc, onkey and regulator)
>>
>> Signed-off-by: Chao Xie <chao.xie-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 841717a..40fd014 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -27,6 +27,7 @@
>>   #include <linux/mfd/core.h>
>>   #include <linux/mfd/88pm80x.h>
>>   #include <linux/slab.h>
>> +#include <linux/of_device.h>
>>
>>   /* Interrupt Registers */
>>   #define PM800_INT_STATUS1		(0x05)
>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>>   };
>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>
>> +static const struct of_device_id pm80x_of_match_table[] = {
>> +	{ .compatible = "marvell,88pm800", },
>> +	{},
>> +};
>> +
>>   static struct resource rtc_resources[] = {
>>   	{
>>   	 .name = "88pm80x-rtc",
>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>   static struct mfd_cell rtc_devs[] = {
>>   	{
>>   	 .name = "88pm80x-rtc",
>> +	 .of_compatible = "marvell,88pm80x-rtc",
>>   	 .num_resources = ARRAY_SIZE(rtc_resources),
>>   	 .resources = &rtc_resources[0],
>>   	 .id = -1,
>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>   static const struct mfd_cell onkey_devs[] = {
>>   	{
>>   	 .name = "88pm80x-onkey",
>> +	 .of_compatible = "marvell,88pm80x-onkey",
>>   	 .num_resources = 1,
>>   	 .resources = &onkey_resources[0],
>>   	 .id = -1,
>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>   static const struct mfd_cell regulator_devs[] = {
>>   	{
>>   	 .name = "88pm80x-regulator",
>> +	 .of_compatible = "marvell,88pm80x-regulator",
>>   	 .id = -1,
>>   	},
>>   };
>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>>   	int ret = 0;
>>   	struct pm80x_chip *chip;
>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>> +	struct device_node *np = client->dev.of_node;
>>   	struct pm80x_subchip *subchip;
>>
>> +	if (!pdata && !np) {
>> +		dev_err(&client->dev,
>> +			"pm80x requires platform data or of_node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!pdata) {
>> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> +		if (!pdata)
>> +			return -ENOMEM;
>> +	}
>
> Why have you allocated data for pdata, then done nothing with it?
>

Not in this patch, but subsequent patches would use it.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> Add DT support to the 88pm800 driver, along with compatible
>> field for it's sub-devices (rtc, onkey and regulator)
>>
>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 841717a..40fd014 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -27,6 +27,7 @@
>>   #include <linux/mfd/core.h>
>>   #include <linux/mfd/88pm80x.h>
>>   #include <linux/slab.h>
>> +#include <linux/of_device.h>
>>
>>   /* Interrupt Registers */
>>   #define PM800_INT_STATUS1		(0x05)
>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>>   };
>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>
>> +static const struct of_device_id pm80x_of_match_table[] = {
>> +	{ .compatible = "marvell,88pm800", },
>> +	{},
>> +};
>> +
>>   static struct resource rtc_resources[] = {
>>   	{
>>   	 .name = "88pm80x-rtc",
>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>   static struct mfd_cell rtc_devs[] = {
>>   	{
>>   	 .name = "88pm80x-rtc",
>> +	 .of_compatible = "marvell,88pm80x-rtc",
>>   	 .num_resources = ARRAY_SIZE(rtc_resources),
>>   	 .resources = &rtc_resources[0],
>>   	 .id = -1,
>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>   static const struct mfd_cell onkey_devs[] = {
>>   	{
>>   	 .name = "88pm80x-onkey",
>> +	 .of_compatible = "marvell,88pm80x-onkey",
>>   	 .num_resources = 1,
>>   	 .resources = &onkey_resources[0],
>>   	 .id = -1,
>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>   static const struct mfd_cell regulator_devs[] = {
>>   	{
>>   	 .name = "88pm80x-regulator",
>> +	 .of_compatible = "marvell,88pm80x-regulator",
>>   	 .id = -1,
>>   	},
>>   };
>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>>   	int ret = 0;
>>   	struct pm80x_chip *chip;
>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>> +	struct device_node *np = client->dev.of_node;
>>   	struct pm80x_subchip *subchip;
>>
>> +	if (!pdata && !np) {
>> +		dev_err(&client->dev,
>> +			"pm80x requires platform data or of_node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!pdata) {
>> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> +		if (!pdata)
>> +			return -ENOMEM;
>> +	}
>
> Why have you allocated data for pdata, then done nothing with it?
>

Not in this patch, but subsequent patches would use it.

Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name.
>>
>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 40fd014..e0cd7ad 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   {
>>   	struct regmap *map = chip->regmap;
>>   	unsigned long flags = IRQF_ONESHOT;
>> -	int data, mask, ret = -EINVAL;
>> +	int irq_clr_mode, mask, ret = -EINVAL;
>>
>>   	if (!map || !chip->irq) {
>>   		dev_err(chip->dev, "incorrect parameters\n");
>> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   	}
>>
>>   	/*
>> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
>> -	 * default.
>> +	 * irq_clr_on_wr defines the way of clearing interrupt by
>> +	 * read/write(0/1).  It's read-clear by default.
>>   	 */
>>   	mask =
>>   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>   	    PM800_WAKEUP2_INT_MASK;
>>
>> -	data = PM800_WAKEUP2_INT_CLEAR;
>> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>> +	irq_clr_mode = (chip->irq_clr_on_wr) ?
>
> Drop the brackets.
>

Ok

>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>
>>   	if (ret < 0)
>>   		goto out;
>> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>   	}
>>
>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>
> You have protection around pdata everywhere else in the file, I
> suggest you supply some here too.
>

Actually it is not really needed, as the PATCH 1/1 introduces


         if (!pdata && !np) {
                 dev_err(&client->dev,
                         "pm80x requires platform data or of_node\n");
                 return -EINVAL;
         }

         if (!pdata && !np) {
                 dev_err(&client->dev,
                         "pm80x requires platform data or of_node\n");
                 return -EINVAL;
         }


So there is no way you can have pdata = NULL beyond this point.


>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>>   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   		if (!pdata)
>>   			return -ENOMEM;
>> +
>> +		/* by default, set irq clear method on write */
>> +		pdata->irq_clr_on_wr = true;
>
> You can save yourself some memory here, by removing this seemingly
> pointless allocation and do this above:
>
>          chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;
>

Yes certainly better way of doing it :)
I will change it.


>>   	}
>>
>>   	ret = pm80x_init(client);
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index 97cb283..94b3dcd 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)
>
> Use the BIT() macro.
>

I thought about this, but the whole file doesn't use it, so I also
chose not to.


>>   #define PM800_POWER_UP_LOG	(0x10)
>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>   	struct regmap_irq_chip_data *irq_data;
>>   	int type;
>>   	int irq;
>> -	int irq_mode;
>> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
>
> Whitespace issue.
>

Didn't see any...and I also ran checkpatch.

> Shouldn't this be a bool?
>

Just was not sure about any older board file interface.
Ideally it should be bool only.

> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
> and call the variable irq_clear_method, or something.
>
> Much clearer that way I think.
>

We have slowly decided to almost hardcode it to one value if there is
no board file. I feel we should just keep it to simple.

If you still insist, I can implement.


Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name.
>>
>> Signed-off-by: Zhao Ye <zhaoy-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 40fd014..e0cd7ad 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   {
>>   	struct regmap *map = chip->regmap;
>>   	unsigned long flags = IRQF_ONESHOT;
>> -	int data, mask, ret = -EINVAL;
>> +	int irq_clr_mode, mask, ret = -EINVAL;
>>
>>   	if (!map || !chip->irq) {
>>   		dev_err(chip->dev, "incorrect parameters\n");
>> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   	}
>>
>>   	/*
>> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
>> -	 * default.
>> +	 * irq_clr_on_wr defines the way of clearing interrupt by
>> +	 * read/write(0/1).  It's read-clear by default.
>>   	 */
>>   	mask =
>>   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>   	    PM800_WAKEUP2_INT_MASK;
>>
>> -	data = PM800_WAKEUP2_INT_CLEAR;
>> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>> +	irq_clr_mode = (chip->irq_clr_on_wr) ?
>
> Drop the brackets.
>

Ok

>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>
>>   	if (ret < 0)
>>   		goto out;
>> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>   	}
>>
>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>
> You have protection around pdata everywhere else in the file, I
> suggest you supply some here too.
>

Actually it is not really needed, as the PATCH 1/1 introduces


         if (!pdata && !np) {
                 dev_err(&client->dev,
                         "pm80x requires platform data or of_node\n");
                 return -EINVAL;
         }

         if (!pdata && !np) {
                 dev_err(&client->dev,
                         "pm80x requires platform data or of_node\n");
                 return -EINVAL;
         }


So there is no way you can have pdata = NULL beyond this point.


>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>>   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   		if (!pdata)
>>   			return -ENOMEM;
>> +
>> +		/* by default, set irq clear method on write */
>> +		pdata->irq_clr_on_wr = true;
>
> You can save yourself some memory here, by removing this seemingly
> pointless allocation and do this above:
>
>          chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;
>

Yes certainly better way of doing it :)
I will change it.


>>   	}
>>
>>   	ret = pm80x_init(client);
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index 97cb283..94b3dcd 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)
>
> Use the BIT() macro.
>

I thought about this, but the whole file doesn't use it, so I also
chose not to.


>>   #define PM800_POWER_UP_LOG	(0x10)
>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>   	struct regmap_irq_chip_data *irq_data;
>>   	int type;
>>   	int irq;
>> -	int irq_mode;
>> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
>
> Whitespace issue.
>

Didn't see any...and I also ran checkpatch.

> Shouldn't this be a bool?
>

Just was not sure about any older board file interface.
Ideally it should be bool only.

> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
> and call the variable irq_clear_method, or something.
>
> Much clearer that way I think.
>

We have slowly decided to almost hardcode it to one value if there is
no board file. I feel we should just keep it to simple.

If you still insist, I can implement.


Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name.
>>
>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 40fd014..e0cd7ad 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   {
>>   	struct regmap *map = chip->regmap;
>>   	unsigned long flags = IRQF_ONESHOT;
>> -	int data, mask, ret = -EINVAL;
>> +	int irq_clr_mode, mask, ret = -EINVAL;
>>
>>   	if (!map || !chip->irq) {
>>   		dev_err(chip->dev, "incorrect parameters\n");
>> @@ -384,15 +384,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   	}
>>
>>   	/*
>> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
>> -	 * default.
>> +	 * irq_clr_on_wr defines the way of clearing interrupt by
>> +	 * read/write(0/1).  It's read-clear by default.
>>   	 */
>>   	mask =
>>   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>   	    PM800_WAKEUP2_INT_MASK;
>>
>> -	data = PM800_WAKEUP2_INT_CLEAR;
>> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>> +	irq_clr_mode = (chip->irq_clr_on_wr) ?
>
> Drop the brackets.
>

Ok

>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>
>>   	if (ret < 0)
>>   		goto out;
>> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>   	}
>>
>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>
> You have protection around pdata everywhere else in the file, I
> suggest you supply some here too.
>

Actually it is not really needed, as the PATCH 1/1 introduces


         if (!pdata && !np) {
                 dev_err(&client->dev,
                         "pm80x requires platform data or of_node\n");
                 return -EINVAL;
         }

         if (!pdata && !np) {
                 dev_err(&client->dev,
                         "pm80x requires platform data or of_node\n");
                 return -EINVAL;
         }


So there is no way you can have pdata = NULL beyond this point.


>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>>   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   		if (!pdata)
>>   			return -ENOMEM;
>> +
>> +		/* by default, set irq clear method on write */
>> +		pdata->irq_clr_on_wr = true;
>
> You can save yourself some memory here, by removing this seemingly
> pointless allocation and do this above:
>
>          chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;
>

Yes certainly better way of doing it :)
I will change it.


>>   	}
>>
>>   	ret = pm80x_init(client);
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index 97cb283..94b3dcd 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)
>
> Use the BIT() macro.
>

I thought about this, but the whole file doesn't use it, so I also
chose not to.


>>   #define PM800_POWER_UP_LOG	(0x10)
>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>   	struct regmap_irq_chip_data *irq_data;
>>   	int type;
>>   	int irq;
>> -	int irq_mode;
>> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
>
> Whitespace issue.
>

Didn't see any...and I also ran checkpatch.

> Shouldn't this be a bool?
>

Just was not sure about any older board file interface.
Ideally it should be bool only.

> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
> and call the variable irq_clear_method, or something.
>
> Much clearer that way I think.
>

We have slowly decided to almost hardcode it to one value if there is
no board file. I feel we should just keep it to simple.

If you still insist, I can implement.


Thanks,
Vaibhav

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:58 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>>   1 file changed, 54 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..f56b751
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,54 @@
>> +* Marvell 88PM8xx Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
>> +- reg : the I2C slave address for the 88pm8xx chip
>> +- interrupts : IRQ line for the 88pm8xx chip
>> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
>> +- #interrupt-cells : should be 1.
>> +		- The cell is the 88pm8xx local IRQ number
>
> Might just be a personal thing, but I would line up the ':', it's much
> easier on the eyes that way.

Ok,

>
>> +88pm8xx family of devices consists of varied group of sub-devices:
>> +
>> +Device		 	Supply Names	 Description
>> +------		 	------------	 -----------
>> +88pm80x-onkey		:		: On key
>> +88pm80x-rtc		:		: RTC
>> +88pm80x-regulator	:		: Regulators
>> +
>> +Note: More device list will follow
>
> No real need for this.
>

I deliberately added it, as I was sure i would get comment if I only
put e devices in the list. And guess what, as expected I got comment.

Anyway, I agree and Will remove it.

Thanks,
Vaibhav

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:58 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>>   1 file changed, 54 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..f56b751
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,54 @@
>> +* Marvell 88PM8xx Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
>> +- reg : the I2C slave address for the 88pm8xx chip
>> +- interrupts : IRQ line for the 88pm8xx chip
>> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
>> +- #interrupt-cells : should be 1.
>> +		- The cell is the 88pm8xx local IRQ number
>
> Might just be a personal thing, but I would line up the ':', it's much
> easier on the eyes that way.

Ok,

>
>> +88pm8xx family of devices consists of varied group of sub-devices:
>> +
>> +Device		 	Supply Names	 Description
>> +------		 	------------	 -----------
>> +88pm80x-onkey		:		: On key
>> +88pm80x-rtc		:		: RTC
>> +88pm80x-regulator	:		: Regulators
>> +
>> +Note: More device list will follow
>
> No real need for this.
>

I deliberately added it, as I was sure i would get comment if I only
put e devices in the list. And guess what, as expected I got comment.

Anyway, I agree and Will remove it.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:58 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>>   1 file changed, 54 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..f56b751
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,54 @@
>> +* Marvell 88PM8xx Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
>> +- reg : the I2C slave address for the 88pm8xx chip
>> +- interrupts : IRQ line for the 88pm8xx chip
>> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
>> +- #interrupt-cells : should be 1.
>> +		- The cell is the 88pm8xx local IRQ number
>
> Might just be a personal thing, but I would line up the ':', it's much
> easier on the eyes that way.

Ok,

>
>> +88pm8xx family of devices consists of varied group of sub-devices:
>> +
>> +Device		 	Supply Names	 Description
>> +------		 	------------	 -----------
>> +88pm80x-onkey		:		: On key
>> +88pm80x-rtc		:		: RTC
>> +88pm80x-regulator	:		: Regulators
>> +
>> +Note: More device list will follow
>
> No real need for this.
>

I deliberately added it, as I was sure i would get comment if I only
put e devices in the list. And guess what, as expected I got comment.

Anyway, I agree and Will remove it.

Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Krzysztof Kozlowski]

2015-06-25 20:19 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>
>
> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:

(...)

>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>> index 97cb283..94b3dcd 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)
>>
>>
>> Use the BIT() macro.
>>
>
> I thought about this, but the whole file doesn't use it, so I also
> chose not to.
>
>
>>>   #define PM800_POWER_UP_LOG    (0x10)
>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>         struct regmap_irq_chip_data *irq_data;
>>>         int type;
>>>         int irq;
>>> -       int irq_mode;
>>> +       int irq_clr_on_wr;      /* '1': Clear on write, '0': Clear on
>>> read*/
>>
>>
>> Whitespace issue.
>>
>
> Didn't see any...and I also ran checkpatch.
>
>> Shouldn't this be a bool?
>>
>
> Just was not sure about any older board file interface.
> Ideally it should be bool only.
>
>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>> and call the variable irq_clear_method, or something.
>>
>> Much clearer that way I think.
>>
>
> We have slowly decided to almost hardcode it to one value if there is
> no board file. I feel we should just keep it to simple.
>
> If you still insist, I can implement.

The bool would be indeed nicer and still you could hard-code the
desired value on DT system:
+               /* by default, set irq clear method on write */
+               pdata->irq_clear_method = CLR_ON_WRITE;

However the question is how this would influence existing platforms
using board files. Are there any in kernel or in downstream?

Best regards,
Krzysztof

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Krzysztof Kozlowski]

2015-06-25 20:19 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
>
>
> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:

(...)

>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>> index 97cb283..94b3dcd 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)
>>
>>
>> Use the BIT() macro.
>>
>
> I thought about this, but the whole file doesn't use it, so I also
> chose not to.
>
>
>>>   #define PM800_POWER_UP_LOG    (0x10)
>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>         struct regmap_irq_chip_data *irq_data;
>>>         int type;
>>>         int irq;
>>> -       int irq_mode;
>>> +       int irq_clr_on_wr;      /* '1': Clear on write, '0': Clear on
>>> read*/
>>
>>
>> Whitespace issue.
>>
>
> Didn't see any...and I also ran checkpatch.
>
>> Shouldn't this be a bool?
>>
>
> Just was not sure about any older board file interface.
> Ideally it should be bool only.
>
>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>> and call the variable irq_clear_method, or something.
>>
>> Much clearer that way I think.
>>
>
> We have slowly decided to almost hardcode it to one value if there is
> no board file. I feel we should just keep it to simple.
>
> If you still insist, I can implement.

The bool would be indeed nicer and still you could hard-code the
desired value on DT system:
+               /* by default, set irq clear method on write */
+               pdata->irq_clear_method = CLR_ON_WRITE;

However the question is how this would influence existing platforms
using board files. Are there any in kernel or in downstream?

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Krzysztof Kozlowski]

2015-06-25 20:19 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>
>
> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:

(...)

>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>> index 97cb283..94b3dcd 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)
>>
>>
>> Use the BIT() macro.
>>
>
> I thought about this, but the whole file doesn't use it, so I also
> chose not to.
>
>
>>>   #define PM800_POWER_UP_LOG    (0x10)
>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>         struct regmap_irq_chip_data *irq_data;
>>>         int type;
>>>         int irq;
>>> -       int irq_mode;
>>> +       int irq_clr_on_wr;      /* '1': Clear on write, '0': Clear on
>>> read*/
>>
>>
>> Whitespace issue.
>>
>
> Didn't see any...and I also ran checkpatch.
>
>> Shouldn't this be a bool?
>>
>
> Just was not sure about any older board file interface.
> Ideally it should be bool only.
>
>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>> and call the variable irq_clear_method, or something.
>>
>> Much clearer that way I think.
>>
>
> We have slowly decided to almost hardcode it to one value if there is
> no board file. I feel we should just keep it to simple.
>
> If you still insist, I can implement.

The bool would be indeed nicer and still you could hard-code the
desired value on DT system:
+               /* by default, set irq clear method on write */
+               pdata->irq_clear_method = CLR_ON_WRITE;

However the question is how this would influence existing platforms
using board files. Are there any in kernel or in downstream?

Best regards,
Krzysztof

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 05:15 PM, Krzysztof Kozlowski wrote:
> 2015-06-25 20:19 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>>
>>
>> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
>
> (...)
>
>>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>>> index 97cb283..94b3dcd 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)
>>>
>>>
>>> Use the BIT() macro.
>>>
>>
>> I thought about this, but the whole file doesn't use it, so I also
>> chose not to.
>>
>>
>>>>    #define PM800_POWER_UP_LOG    (0x10)
>>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>>          struct regmap_irq_chip_data *irq_data;
>>>>          int type;
>>>>          int irq;
>>>> -       int irq_mode;
>>>> +       int irq_clr_on_wr;      /* '1': Clear on write, '0': Clear on
>>>> read*/
>>>
>>>
>>> Whitespace issue.
>>>
>>
>> Didn't see any...and I also ran checkpatch.
>>
>>> Shouldn't this be a bool?
>>>
>>
>> Just was not sure about any older board file interface.
>> Ideally it should be bool only.
>>
>>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>>> and call the variable irq_clear_method, or something.
>>>
>>> Much clearer that way I think.
>>>
>>
>> We have slowly decided to almost hardcode it to one value if there is
>> no board file. I feel we should just keep it to simple.
>>
>> If you still insist, I can implement.
>
> The bool would be indeed nicer and still you could hard-code the
> desired value on DT system:
> +               /* by default, set irq clear method on write */
> +               pdata->irq_clear_method = CLR_ON_WRITE;
>
> However the question is how this would influence existing platforms
> using board files. Are there any in kernel or in downstream?
>

No, not atleast I am aware of.
I did grep on mainline kernel, and here is what I got


drivers/regulator/88pm800.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(pdev->dev.parent);
drivers/mfd/88pm800.c:				       struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:					   struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				     struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(&client->dev);
drivers/mfd/88pm805.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(&client->dev);
include/linux/mfd/88pm80x.h:struct pm80x_platform_data {
include/linux/mfd/88pm80x.h:				struct pm80x_platform_data *pdata);


Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 05:15 PM, Krzysztof Kozlowski wrote:
> 2015-06-25 20:19 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
>>
>>
>> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
>
> (...)
>
>>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>>> index 97cb283..94b3dcd 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)
>>>
>>>
>>> Use the BIT() macro.
>>>
>>
>> I thought about this, but the whole file doesn't use it, so I also
>> chose not to.
>>
>>
>>>>    #define PM800_POWER_UP_LOG    (0x10)
>>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>>          struct regmap_irq_chip_data *irq_data;
>>>>          int type;
>>>>          int irq;
>>>> -       int irq_mode;
>>>> +       int irq_clr_on_wr;      /* '1': Clear on write, '0': Clear on
>>>> read*/
>>>
>>>
>>> Whitespace issue.
>>>
>>
>> Didn't see any...and I also ran checkpatch.
>>
>>> Shouldn't this be a bool?
>>>
>>
>> Just was not sure about any older board file interface.
>> Ideally it should be bool only.
>>
>>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>>> and call the variable irq_clear_method, or something.
>>>
>>> Much clearer that way I think.
>>>
>>
>> We have slowly decided to almost hardcode it to one value if there is
>> no board file. I feel we should just keep it to simple.
>>
>> If you still insist, I can implement.
>
> The bool would be indeed nicer and still you could hard-code the
> desired value on DT system:
> +               /* by default, set irq clear method on write */
> +               pdata->irq_clear_method = CLR_ON_WRITE;
>
> However the question is how this would influence existing platforms
> using board files. Are there any in kernel or in downstream?
>

No, not atleast I am aware of.
I did grep on mainline kernel, and here is what I got


drivers/regulator/88pm800.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(pdev->dev.parent);
drivers/mfd/88pm800.c:				       struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:					   struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				     struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(&client->dev);
drivers/mfd/88pm805.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(&client->dev);
include/linux/mfd/88pm80x.h:struct pm80x_platform_data {
include/linux/mfd/88pm80x.h:				struct pm80x_platform_data *pdata);


Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 05:15 PM, Krzysztof Kozlowski wrote:
> 2015-06-25 20:19 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>>
>>
>> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
>
> (...)
>
>>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>>> index 97cb283..94b3dcd 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)
>>>
>>>
>>> Use the BIT() macro.
>>>
>>
>> I thought about this, but the whole file doesn't use it, so I also
>> chose not to.
>>
>>
>>>>    #define PM800_POWER_UP_LOG    (0x10)
>>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>>          struct regmap_irq_chip_data *irq_data;
>>>>          int type;
>>>>          int irq;
>>>> -       int irq_mode;
>>>> +       int irq_clr_on_wr;      /* '1': Clear on write, '0': Clear on
>>>> read*/
>>>
>>>
>>> Whitespace issue.
>>>
>>
>> Didn't see any...and I also ran checkpatch.
>>
>>> Shouldn't this be a bool?
>>>
>>
>> Just was not sure about any older board file interface.
>> Ideally it should be bool only.
>>
>>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>>> and call the variable irq_clear_method, or something.
>>>
>>> Much clearer that way I think.
>>>
>>
>> We have slowly decided to almost hardcode it to one value if there is
>> no board file. I feel we should just keep it to simple.
>>
>> If you still insist, I can implement.
>
> The bool would be indeed nicer and still you could hard-code the
> desired value on DT system:
> +               /* by default, set irq clear method on write */
> +               pdata->irq_clear_method = CLR_ON_WRITE;
>
> However the question is how this would influence existing platforms
> using board files. Are there any in kernel or in downstream?
>

No, not atleast I am aware of.
I did grep on mainline kernel, and here is what I got


drivers/regulator/88pm800.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(pdev->dev.parent);
drivers/mfd/88pm800.c:				       struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:					   struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:				     struct pm80x_platform_data *pdata)
drivers/mfd/88pm800.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(&client->dev);
drivers/mfd/88pm805.c:	struct pm80x_platform_data *pdata = 
dev_get_platdata(&client->dev);
include/linux/mfd/88pm80x.h:struct pm80x_platform_data {
include/linux/mfd/88pm80x.h:				struct pm80x_platform_data *pdata);


Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >
> >>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>(page 0) controls the method of clearing interrupt
> >>status of 88pm800 family of devices;
> >>
> >>   0: clear on read
> >>   1: clear on write
> >>
> >>If pdata is not coming from board file, then set the
> >>default irq clear method to "irq clear on write"
> >>
> >>Also, as suggested by "Lee Jones" renaming variable field
> >>to appropriate name.
> >>
> >>Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> >>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>---
> >>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >>  include/linux/mfd/88pm80x.h |  6 ++++--
> >>  2 files changed, 14 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index 40fd014..e0cd7ad 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c

[...]

> >>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >>
> >>  	if (ret < 0)
> >>  		goto out;
> >>@@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
> >>  	}
> >>
> >>  	chip->regmap_irq_chip = &pm800_irq_chip;
> >>+	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
> >
> >You have protection around pdata everywhere else in the file, I
> >suggest you supply some here too.
> >
> 
> Actually it is not really needed, as the PATCH 1/1 introduces
> 
> 
>         if (!pdata && !np) {
>                 dev_err(&client->dev,
>                         "pm80x requires platform data or of_node\n");
>                 return -EINVAL;
>         }
> 
>         if (!pdata && !np) {
>                 dev_err(&client->dev,
>                         "pm80x requires platform data or of_node\n");
>                 return -EINVAL;
>         }
> 
> 
> So there is no way you can have pdata = NULL beyond this point.

I saw that.  I want you to remove that too.

[...]

> >>--- 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)
> >
> >Use the BIT() macro.
> >
> 
> I thought about this, but the whole file doesn't use it, so I also
> chose not to.

Then the whole file needs moving over.

Patches accepted.

> >>  #define PM800_POWER_UP_LOG	(0x10)
> >>@@ -300,7 +302,7 @@ struct pm80x_chip {
> >>  	struct regmap_irq_chip_data *irq_data;
> >>  	int type;
> >>  	int irq;
> >>-	int irq_mode;
> >>+	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
> >
> >Whitespace issue.
> 
> Didn't see any...and I also ran checkpatch.

You have no space before the '*/'.

> >Shouldn't this be a bool?
> >
> 
> Just was not sure about any older board file interface.
> Ideally it should be bool only.

Right.

> >Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
> >and call the variable irq_clear_method, or something.
> >
> >Much clearer that way I think.
> >
> 
> We have slowly decided to almost hardcode it to one value if there is
> no board file. I feel we should just keep it to simple.
> 
> If you still insist, I can implement.

I like clarity and by your own admission (by warranting an additional
comment) it's not clear.  Please make it as clear as you can.

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

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >
> >>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>(page 0) controls the method of clearing interrupt
> >>status of 88pm800 family of devices;
> >>
> >>   0: clear on read
> >>   1: clear on write
> >>
> >>If pdata is not coming from board file, then set the
> >>default irq clear method to "irq clear on write"
> >>
> >>Also, as suggested by "Lee Jones" renaming variable field
> >>to appropriate name.
> >>
> >>Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> >>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>---
> >>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >>  include/linux/mfd/88pm80x.h |  6 ++++--
> >>  2 files changed, 14 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index 40fd014..e0cd7ad 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c

[...]

> >>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >>
> >>  	if (ret < 0)
> >>  		goto out;
> >>@@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
> >>  	}
> >>
> >>  	chip->regmap_irq_chip = &pm800_irq_chip;
> >>+	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
> >
> >You have protection around pdata everywhere else in the file, I
> >suggest you supply some here too.
> >
> 
> Actually it is not really needed, as the PATCH 1/1 introduces
> 
> 
>         if (!pdata && !np) {
>                 dev_err(&client->dev,
>                         "pm80x requires platform data or of_node\n");
>                 return -EINVAL;
>         }
> 
>         if (!pdata && !np) {
>                 dev_err(&client->dev,
>                         "pm80x requires platform data or of_node\n");
>                 return -EINVAL;
>         }
> 
> 
> So there is no way you can have pdata = NULL beyond this point.

I saw that.  I want you to remove that too.

[...]

> >>--- 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)
> >
> >Use the BIT() macro.
> >
> 
> I thought about this, but the whole file doesn't use it, so I also
> chose not to.

Then the whole file needs moving over.

Patches accepted.

> >>  #define PM800_POWER_UP_LOG	(0x10)
> >>@@ -300,7 +302,7 @@ struct pm80x_chip {
> >>  	struct regmap_irq_chip_data *irq_data;
> >>  	int type;
> >>  	int irq;
> >>-	int irq_mode;
> >>+	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
> >
> >Whitespace issue.
> 
> Didn't see any...and I also ran checkpatch.

You have no space before the '*/'.

> >Shouldn't this be a bool?
> >
> 
> Just was not sure about any older board file interface.
> Ideally it should be bool only.

Right.

> >Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
> >and call the variable irq_clear_method, or something.
> >
> >Much clearer that way I think.
> >
> 
> We have slowly decided to almost hardcode it to one value if there is
> no board file. I feel we should just keep it to simple.
> 
> If you still insist, I can implement.

I like clarity and by your own admission (by warranting an additional
comment) it's not clear.  Please make it as clear as you can.

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

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >
> >>Add DT support to the 88pm800 driver, along with compatible
> >>field for it's sub-devices (rtc, onkey and regulator)
> >>
> >>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>---
> >>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index 841717a..40fd014 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c
> >>@@ -27,6 +27,7 @@
> >>  #include <linux/mfd/core.h>
> >>  #include <linux/mfd/88pm80x.h>
> >>  #include <linux/slab.h>
> >>+#include <linux/of_device.h>
> >>
> >>  /* Interrupt Registers */
> >>  #define PM800_INT_STATUS1		(0x05)
> >>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>  };
> >>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>
> >>+static const struct of_device_id pm80x_of_match_table[] = {
> >>+	{ .compatible = "marvell,88pm800", },
> >>+	{},
> >>+};
> >>+
> >>  static struct resource rtc_resources[] = {
> >>  	{
> >>  	 .name = "88pm80x-rtc",
> >>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>  static struct mfd_cell rtc_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-rtc",
> >>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>  	 .resources = &rtc_resources[0],
> >>  	 .id = -1,
> >>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>  static const struct mfd_cell onkey_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-onkey",
> >>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>  	 .num_resources = 1,
> >>  	 .resources = &onkey_resources[0],
> >>  	 .id = -1,
> >>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>  static const struct mfd_cell regulator_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-regulator",
> >>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>  	 .id = -1,
> >>  	},
> >>  };
> >>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>  	int ret = 0;
> >>  	struct pm80x_chip *chip;
> >>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>+	struct device_node *np = client->dev.of_node;
> >>  	struct pm80x_subchip *subchip;
> >>
> >>+	if (!pdata && !np) {
> >>+		dev_err(&client->dev,
> >>+			"pm80x requires platform data or of_node\n");
> >>+		return -EINVAL;
> >>+	}
> >>+
> >>+	if (!pdata) {
> >>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>+		if (!pdata)
> >>+			return -ENOMEM;
> >>+	}
> >
> >Why have you allocated data for pdata, then done nothing with it?
> >
> 
> Not in this patch, but subsequent patches would use it.

Only provide it when you start using it please.

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

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >
> >>Add DT support to the 88pm800 driver, along with compatible
> >>field for it's sub-devices (rtc, onkey and regulator)
> >>
> >>Signed-off-by: Chao Xie <chao.xie-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>---
> >>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index 841717a..40fd014 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c
> >>@@ -27,6 +27,7 @@
> >>  #include <linux/mfd/core.h>
> >>  #include <linux/mfd/88pm80x.h>
> >>  #include <linux/slab.h>
> >>+#include <linux/of_device.h>
> >>
> >>  /* Interrupt Registers */
> >>  #define PM800_INT_STATUS1		(0x05)
> >>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>  };
> >>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>
> >>+static const struct of_device_id pm80x_of_match_table[] = {
> >>+	{ .compatible = "marvell,88pm800", },
> >>+	{},
> >>+};
> >>+
> >>  static struct resource rtc_resources[] = {
> >>  	{
> >>  	 .name = "88pm80x-rtc",
> >>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>  static struct mfd_cell rtc_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-rtc",
> >>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>  	 .resources = &rtc_resources[0],
> >>  	 .id = -1,
> >>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>  static const struct mfd_cell onkey_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-onkey",
> >>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>  	 .num_resources = 1,
> >>  	 .resources = &onkey_resources[0],
> >>  	 .id = -1,
> >>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>  static const struct mfd_cell regulator_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-regulator",
> >>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>  	 .id = -1,
> >>  	},
> >>  };
> >>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>  	int ret = 0;
> >>  	struct pm80x_chip *chip;
> >>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>+	struct device_node *np = client->dev.of_node;
> >>  	struct pm80x_subchip *subchip;
> >>
> >>+	if (!pdata && !np) {
> >>+		dev_err(&client->dev,
> >>+			"pm80x requires platform data or of_node\n");
> >>+		return -EINVAL;
> >>+	}
> >>+
> >>+	if (!pdata) {
> >>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>+		if (!pdata)
> >>+			return -ENOMEM;
> >>+	}
> >
> >Why have you allocated data for pdata, then done nothing with it?
> >
> 
> Not in this patch, but subsequent patches would use it.

Only provide it when you start using it please.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Lee Jones]

On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >
> >>Add DT support to the 88pm800 driver, along with compatible
> >>field for it's sub-devices (rtc, onkey and regulator)
> >>
> >>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>---
> >>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index 841717a..40fd014 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c
> >>@@ -27,6 +27,7 @@
> >>  #include <linux/mfd/core.h>
> >>  #include <linux/mfd/88pm80x.h>
> >>  #include <linux/slab.h>
> >>+#include <linux/of_device.h>
> >>
> >>  /* Interrupt Registers */
> >>  #define PM800_INT_STATUS1		(0x05)
> >>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>  };
> >>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>
> >>+static const struct of_device_id pm80x_of_match_table[] = {
> >>+	{ .compatible = "marvell,88pm800", },
> >>+	{},
> >>+};
> >>+
> >>  static struct resource rtc_resources[] = {
> >>  	{
> >>  	 .name = "88pm80x-rtc",
> >>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>  static struct mfd_cell rtc_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-rtc",
> >>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>  	 .resources = &rtc_resources[0],
> >>  	 .id = -1,
> >>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>  static const struct mfd_cell onkey_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-onkey",
> >>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>  	 .num_resources = 1,
> >>  	 .resources = &onkey_resources[0],
> >>  	 .id = -1,
> >>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>  static const struct mfd_cell regulator_devs[] = {
> >>  	{
> >>  	 .name = "88pm80x-regulator",
> >>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>  	 .id = -1,
> >>  	},
> >>  };
> >>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>  	int ret = 0;
> >>  	struct pm80x_chip *chip;
> >>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>+	struct device_node *np = client->dev.of_node;
> >>  	struct pm80x_subchip *subchip;
> >>
> >>+	if (!pdata && !np) {
> >>+		dev_err(&client->dev,
> >>+			"pm80x requires platform data or of_node\n");
> >>+		return -EINVAL;
> >>+	}
> >>+
> >>+	if (!pdata) {
> >>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>+		if (!pdata)
> >>+			return -ENOMEM;
> >>+	}
> >
> >Why have you allocated data for pdata, then done nothing with it?
> >
> 
> Not in this patch, but subsequent patches would use it.

Only provide it when you start using it please.

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

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>
>>>> Add DT support to the 88pm800 driver, along with compatible
>>>> field for it's sub-devices (rtc, onkey and regulator)
>>>>
>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> ---
>>>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>>>   1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index 841717a..40fd014 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>>>> @@ -27,6 +27,7 @@
>>>>   #include <linux/mfd/core.h>
>>>>   #include <linux/mfd/88pm80x.h>
>>>>   #include <linux/slab.h>
>>>> +#include <linux/of_device.h>
>>>>
>>>>   /* Interrupt Registers */
>>>>   #define PM800_INT_STATUS1		(0x05)
>>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>>>>   };
>>>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>>
>>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>>> +	{ .compatible = "marvell,88pm800", },
>>>> +	{},
>>>> +};
>>>> +
>>>>   static struct resource rtc_resources[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-rtc",
>>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>>>   static struct mfd_cell rtc_devs[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-rtc",
>>>> +	 .of_compatible = "marvell,88pm80x-rtc",
>>>>   	 .num_resources = ARRAY_SIZE(rtc_resources),
>>>>   	 .resources = &rtc_resources[0],
>>>>   	 .id = -1,
>>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>>>   static const struct mfd_cell onkey_devs[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-onkey",
>>>> +	 .of_compatible = "marvell,88pm80x-onkey",
>>>>   	 .num_resources = 1,
>>>>   	 .resources = &onkey_resources[0],
>>>>   	 .id = -1,
>>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>>>   static const struct mfd_cell regulator_devs[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-regulator",
>>>> +	 .of_compatible = "marvell,88pm80x-regulator",
>>>>   	 .id = -1,
>>>>   	},
>>>>   };
>>>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>>>>   	int ret = 0;
>>>>   	struct pm80x_chip *chip;
>>>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>>>> +	struct device_node *np = client->dev.of_node;
>>>>   	struct pm80x_subchip *subchip;
>>>>
>>>> +	if (!pdata && !np) {
>>>> +		dev_err(&client->dev,
>>>> +			"pm80x requires platform data or of_node\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (!pdata) {
>>>> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>>> +		if (!pdata)
>>>> +			return -ENOMEM;
>>>> +	}
>>>
>>> Why have you allocated data for pdata, then done nothing with it?
>>>
>>
>> Not in this patch, but subsequent patches would use it.
>
> Only provide it when you start using it please.
>

I will take back my earlier comment of "not using in this patch, but
subsequent patches".

pdata is being used, couple of places in the driver,


@line-751

         ret = device_800_init(chip, pdata);
         if (ret) {
                 dev_err(chip->dev, "Failed to initialize 88pm800 
devices\n");
                 goto err_device_init;
         }

         if (pdata && pdata->plat_config)
                 pdata->plat_config(chip, pdata);

Thanks,
Vaibhav

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>
>>>> Add DT support to the 88pm800 driver, along with compatible
>>>> field for it's sub-devices (rtc, onkey and regulator)
>>>>
>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> ---
>>>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>>>   1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index 841717a..40fd014 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>>>> @@ -27,6 +27,7 @@
>>>>   #include <linux/mfd/core.h>
>>>>   #include <linux/mfd/88pm80x.h>
>>>>   #include <linux/slab.h>
>>>> +#include <linux/of_device.h>
>>>>
>>>>   /* Interrupt Registers */
>>>>   #define PM800_INT_STATUS1		(0x05)
>>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>>>>   };
>>>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>>
>>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>>> +	{ .compatible = "marvell,88pm800", },
>>>> +	{},
>>>> +};
>>>> +
>>>>   static struct resource rtc_resources[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-rtc",
>>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>>>   static struct mfd_cell rtc_devs[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-rtc",
>>>> +	 .of_compatible = "marvell,88pm80x-rtc",
>>>>   	 .num_resources = ARRAY_SIZE(rtc_resources),
>>>>   	 .resources = &rtc_resources[0],
>>>>   	 .id = -1,
>>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>>>   static const struct mfd_cell onkey_devs[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-onkey",
>>>> +	 .of_compatible = "marvell,88pm80x-onkey",
>>>>   	 .num_resources = 1,
>>>>   	 .resources = &onkey_resources[0],
>>>>   	 .id = -1,
>>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>>>   static const struct mfd_cell regulator_devs[] = {
>>>>   	{
>>>>   	 .name = "88pm80x-regulator",
>>>> +	 .of_compatible = "marvell,88pm80x-regulator",
>>>>   	 .id = -1,
>>>>   	},
>>>>   };
>>>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>>>>   	int ret = 0;
>>>>   	struct pm80x_chip *chip;
>>>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>>>> +	struct device_node *np = client->dev.of_node;
>>>>   	struct pm80x_subchip *subchip;
>>>>
>>>> +	if (!pdata && !np) {
>>>> +		dev_err(&client->dev,
>>>> +			"pm80x requires platform data or of_node\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (!pdata) {
>>>> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>>> +		if (!pdata)
>>>> +			return -ENOMEM;
>>>> +	}
>>>
>>> Why have you allocated data for pdata, then done nothing with it?
>>>
>>
>> Not in this patch, but subsequent patches would use it.
>
> Only provide it when you start using it please.
>

I will take back my earlier comment of "not using in this patch, but
subsequent patches".

pdata is being used, couple of places in the driver,


@line-751

         ret = device_800_init(chip, pdata);
         if (ret) {
                 dev_err(chip->dev, "Failed to initialize 88pm800 
devices\n");
                 goto err_device_init;
         }

         if (pdata && pdata->plat_config)
                 pdata->plat_config(chip, pdata);

Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 08:16 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>
>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>> (page 0) controls the method of clearing interrupt
>>>> status of 88pm800 family of devices;
>>>>
>>>>    0: clear on read
>>>>    1: clear on write
>>>>
>>>> If pdata is not coming from board file, then set the
>>>> default irq clear method to "irq clear on write"
>>>>
>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>> to appropriate name.
>>>>
>>>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> ---
>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index 40fd014..e0cd7ad 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>
> [...]
>
>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>>
>>>>   	if (ret < 0)
>>>>   		goto out;
>>>> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>>>   	}
>>>>
>>>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>>>> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>>>
>>> You have protection around pdata everywhere else in the file, I
>>> suggest you supply some here too.
>>>
>>
>> Actually it is not really needed, as the PATCH 1/1 introduces
>>
>>
>>          if (!pdata && !np) {
>>                  dev_err(&client->dev,
>>                          "pm80x requires platform data or of_node\n");
>>                  return -EINVAL;
>>          }
>>
>>          if (!pdata && !np) {
>>                  dev_err(&client->dev,
>>                          "pm80x requires platform data or of_node\n");
>>                  return -EINVAL;
>>          }
>>
>>
>> So there is no way you can have pdata = NULL beyond this point.
>
> I saw that.  I want you to remove that too.
>

You mean, remove existing protection in the driver?
I will create a separate patch for this.

> [...]
>
>>>> --- 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)
>>>
>>> Use the BIT() macro.
>>>
>>
>> I thought about this, but the whole file doesn't use it, so I also
>> chose not to.
>
> Then the whole file needs moving over.
>
> Patches accepted.
>

Will change the driver and submit the patch.


>>>>   #define PM800_POWER_UP_LOG	(0x10)
>>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>>   	struct regmap_irq_chip_data *irq_data;
>>>>   	int type;
>>>>   	int irq;
>>>> -	int irq_mode;
>>>> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
>>>
>>> Whitespace issue.
>>
>> Didn't see any...and I also ran checkpatch.
>
> You have no space before the '*/'.
>
>>> Shouldn't this be a bool?
>>>
>>
>> Just was not sure about any older board file interface.
>> Ideally it should be bool only.
>
> Right.
>
>>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>>> and call the variable irq_clear_method, or something.
>>>
>>> Much clearer that way I think.
>>>
>>
>> We have slowly decided to almost hardcode it to one value if there is
>> no board file. I feel we should just keep it to simple.
>>
>> If you still insist, I can implement.
>
> I like clarity and by your own admission (by warranting an additional
> comment) it's not clear.  Please make it as clear as you can.
>

OK, will change the code for CLEAR_ON_READ and CLEAR_ON_WRITE.

Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 08:16 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>
>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>> (page 0) controls the method of clearing interrupt
>>>> status of 88pm800 family of devices;
>>>>
>>>>    0: clear on read
>>>>    1: clear on write
>>>>
>>>> If pdata is not coming from board file, then set the
>>>> default irq clear method to "irq clear on write"
>>>>
>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>> to appropriate name.
>>>>
>>>> Signed-off-by: Zhao Ye <zhaoy-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> ---
>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index 40fd014..e0cd7ad 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>
> [...]
>
>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>>
>>>>   	if (ret < 0)
>>>>   		goto out;
>>>> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>>>   	}
>>>>
>>>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>>>> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>>>
>>> You have protection around pdata everywhere else in the file, I
>>> suggest you supply some here too.
>>>
>>
>> Actually it is not really needed, as the PATCH 1/1 introduces
>>
>>
>>          if (!pdata && !np) {
>>                  dev_err(&client->dev,
>>                          "pm80x requires platform data or of_node\n");
>>                  return -EINVAL;
>>          }
>>
>>          if (!pdata && !np) {
>>                  dev_err(&client->dev,
>>                          "pm80x requires platform data or of_node\n");
>>                  return -EINVAL;
>>          }
>>
>>
>> So there is no way you can have pdata = NULL beyond this point.
>
> I saw that.  I want you to remove that too.
>

You mean, remove existing protection in the driver?
I will create a separate patch for this.

> [...]
>
>>>> --- 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)
>>>
>>> Use the BIT() macro.
>>>
>>
>> I thought about this, but the whole file doesn't use it, so I also
>> chose not to.
>
> Then the whole file needs moving over.
>
> Patches accepted.
>

Will change the driver and submit the patch.


>>>>   #define PM800_POWER_UP_LOG	(0x10)
>>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>>   	struct regmap_irq_chip_data *irq_data;
>>>>   	int type;
>>>>   	int irq;
>>>> -	int irq_mode;
>>>> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
>>>
>>> Whitespace issue.
>>
>> Didn't see any...and I also ran checkpatch.
>
> You have no space before the '*/'.
>
>>> Shouldn't this be a bool?
>>>
>>
>> Just was not sure about any older board file interface.
>> Ideally it should be bool only.
>
> Right.
>
>>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>>> and call the variable irq_clear_method, or something.
>>>
>>> Much clearer that way I think.
>>>
>>
>> We have slowly decided to almost hardcode it to one value if there is
>> no board file. I feel we should just keep it to simple.
>>
>> If you still insist, I can implement.
>
> I like clarity and by your own admission (by warranting an additional
> comment) it's not clear.  Please make it as clear as you can.
>

OK, will change the code for CLEAR_ON_READ and CLEAR_ON_WRITE.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 08:16 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>> On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>
>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>> (page 0) controls the method of clearing interrupt
>>>> status of 88pm800 family of devices;
>>>>
>>>>    0: clear on read
>>>>    1: clear on write
>>>>
>>>> If pdata is not coming from board file, then set the
>>>> default irq clear method to "irq clear on write"
>>>>
>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>> to appropriate name.
>>>>
>>>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> ---
>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index 40fd014..e0cd7ad 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>
> [...]
>
>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>>
>>>>   	if (ret < 0)
>>>>   		goto out;
>>>> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>>>   	}
>>>>
>>>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>>>> +	chip->irq_clr_on_wr = pdata->irq_clr_on_wr;
>>>
>>> You have protection around pdata everywhere else in the file, I
>>> suggest you supply some here too.
>>>
>>
>> Actually it is not really needed, as the PATCH 1/1 introduces
>>
>>
>>          if (!pdata && !np) {
>>                  dev_err(&client->dev,
>>                          "pm80x requires platform data or of_node\n");
>>                  return -EINVAL;
>>          }
>>
>>          if (!pdata && !np) {
>>                  dev_err(&client->dev,
>>                          "pm80x requires platform data or of_node\n");
>>                  return -EINVAL;
>>          }
>>
>>
>> So there is no way you can have pdata = NULL beyond this point.
>
> I saw that.  I want you to remove that too.
>

You mean, remove existing protection in the driver?
I will create a separate patch for this.

> [...]
>
>>>> --- 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)
>>>
>>> Use the BIT() macro.
>>>
>>
>> I thought about this, but the whole file doesn't use it, so I also
>> chose not to.
>
> Then the whole file needs moving over.
>
> Patches accepted.
>

Will change the driver and submit the patch.


>>>>   #define PM800_POWER_UP_LOG	(0x10)
>>>> @@ -300,7 +302,7 @@ struct pm80x_chip {
>>>>   	struct regmap_irq_chip_data *irq_data;
>>>>   	int type;
>>>>   	int irq;
>>>> -	int irq_mode;
>>>> +	int irq_clr_on_wr;	/* '1': Clear on write, '0': Clear on read*/
>>>
>>> Whitespace issue.
>>
>> Didn't see any...and I also ran checkpatch.
>
> You have no space before the '*/'.
>
>>> Shouldn't this be a bool?
>>>
>>
>> Just was not sure about any older board file interface.
>> Ideally it should be bool only.
>
> Right.
>
>>> Actually even better, I would define; CLR_ON_WRITE and CLR_ON_READ,
>>> and call the variable irq_clear_method, or something.
>>>
>>> Much clearer that way I think.
>>>
>>
>> We have slowly decided to almost hardcode it to one value if there is
>> no board file. I feel we should just keep it to simple.
>>
>> If you still insist, I can implement.
>
> I like clarity and by your own admission (by warranting an additional
> comment) it's not clear.  Please make it as clear as you can.
>

OK, will change the code for CLEAR_ON_READ and CLEAR_ON_WRITE.

Thanks,
Vaibhav

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Yi Zhang]

On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>
> >>>>Add DT support to the 88pm800 driver, along with compatible
> >>>>field for it's sub-devices (rtc, onkey and regulator)
> >>>>
> >>>>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>---
> >>>>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>>>  1 file changed, 23 insertions(+)
> >>>>
> >>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>index 841717a..40fd014 100644
> >>>>--- a/drivers/mfd/88pm800.c
> >>>>+++ b/drivers/mfd/88pm800.c
> >>>>@@ -27,6 +27,7 @@
> >>>>  #include <linux/mfd/core.h>
> >>>>  #include <linux/mfd/88pm80x.h>
> >>>>  #include <linux/slab.h>
> >>>>+#include <linux/of_device.h>
> >>>>
> >>>>  /* Interrupt Registers */
> >>>>  #define PM800_INT_STATUS1		(0x05)
> >>>>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>>>  };
> >>>>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>>>
> >>>>+static const struct of_device_id pm80x_of_match_table[] = {
> >>>>+	{ .compatible = "marvell,88pm800", },
> >>>>+	{},
> >>>>+};
> >>>>+
> >>>>  static struct resource rtc_resources[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-rtc",
> >>>>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>>>  static struct mfd_cell rtc_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-rtc",
> >>>>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>>>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>>>  	 .resources = &rtc_resources[0],
> >>>>  	 .id = -1,
> >>>>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>>>  static const struct mfd_cell onkey_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-onkey",
> >>>>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>>>  	 .num_resources = 1,
> >>>>  	 .resources = &onkey_resources[0],
> >>>>  	 .id = -1,
> >>>>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>>>  static const struct mfd_cell regulator_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-regulator",
> >>>>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>>>  	 .id = -1,
> >>>>  	},
> >>>>  };
> >>>>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>>>  	int ret = 0;
> >>>>  	struct pm80x_chip *chip;
> >>>>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>>>+	struct device_node *np = client->dev.of_node;
> >>>>  	struct pm80x_subchip *subchip;
> >>>>
> >>>>+	if (!pdata && !np) {
> >>>>+		dev_err(&client->dev,
> >>>>+			"pm80x requires platform data or of_node\n");
> >>>>+		return -EINVAL;
> >>>>+	}
> >>>>+
> >>>>+	if (!pdata) {
> >>>>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>>>+		if (!pdata)
> >>>>+			return -ENOMEM;
> >>>>+	}
> >>>
> >>>Why have you allocated data for pdata, then done nothing with it?
> >>>
> >>
> >>Not in this patch, but subsequent patches would use it.
> >
> >Only provide it when you start using it please.
> >
> 
> I will take back my earlier comment of "not using in this patch, but
> subsequent patches".
> 
> pdata is being used, couple of places in the driver,
> 
> 
> @line-751
> 
>         ret = device_800_init(chip, pdata);
>         if (ret) {
>                 dev_err(chip->dev, "Failed to initialize 88pm800
> devices\n");
>                 goto err_device_init;
>         }
> 
>         if (pdata && pdata->plat_config)
>                 pdata->plat_config(chip, pdata);

  this plat_config() is used in legacy non-device-tree code, it's used
  to implement fixup for chip or board level, it exists in
  the board configuration file

  just curious, do you think we still need to keep it?
  considering device-tree has been used. thanks;

> 
> Thanks,
> Vaibhav

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Yi Zhang]

On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>
> >>>>Add DT support to the 88pm800 driver, along with compatible
> >>>>field for it's sub-devices (rtc, onkey and regulator)
> >>>>
> >>>>Signed-off-by: Chao Xie <chao.xie-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>---
> >>>>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>>>  1 file changed, 23 insertions(+)
> >>>>
> >>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>index 841717a..40fd014 100644
> >>>>--- a/drivers/mfd/88pm800.c
> >>>>+++ b/drivers/mfd/88pm800.c
> >>>>@@ -27,6 +27,7 @@
> >>>>  #include <linux/mfd/core.h>
> >>>>  #include <linux/mfd/88pm80x.h>
> >>>>  #include <linux/slab.h>
> >>>>+#include <linux/of_device.h>
> >>>>
> >>>>  /* Interrupt Registers */
> >>>>  #define PM800_INT_STATUS1		(0x05)
> >>>>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>>>  };
> >>>>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>>>
> >>>>+static const struct of_device_id pm80x_of_match_table[] = {
> >>>>+	{ .compatible = "marvell,88pm800", },
> >>>>+	{},
> >>>>+};
> >>>>+
> >>>>  static struct resource rtc_resources[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-rtc",
> >>>>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>>>  static struct mfd_cell rtc_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-rtc",
> >>>>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>>>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>>>  	 .resources = &rtc_resources[0],
> >>>>  	 .id = -1,
> >>>>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>>>  static const struct mfd_cell onkey_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-onkey",
> >>>>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>>>  	 .num_resources = 1,
> >>>>  	 .resources = &onkey_resources[0],
> >>>>  	 .id = -1,
> >>>>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>>>  static const struct mfd_cell regulator_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-regulator",
> >>>>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>>>  	 .id = -1,
> >>>>  	},
> >>>>  };
> >>>>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>>>  	int ret = 0;
> >>>>  	struct pm80x_chip *chip;
> >>>>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>>>+	struct device_node *np = client->dev.of_node;
> >>>>  	struct pm80x_subchip *subchip;
> >>>>
> >>>>+	if (!pdata && !np) {
> >>>>+		dev_err(&client->dev,
> >>>>+			"pm80x requires platform data or of_node\n");
> >>>>+		return -EINVAL;
> >>>>+	}
> >>>>+
> >>>>+	if (!pdata) {
> >>>>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>>>+		if (!pdata)
> >>>>+			return -ENOMEM;
> >>>>+	}
> >>>
> >>>Why have you allocated data for pdata, then done nothing with it?
> >>>
> >>
> >>Not in this patch, but subsequent patches would use it.
> >
> >Only provide it when you start using it please.
> >
> 
> I will take back my earlier comment of "not using in this patch, but
> subsequent patches".
> 
> pdata is being used, couple of places in the driver,
> 
> 
> @line-751
> 
>         ret = device_800_init(chip, pdata);
>         if (ret) {
>                 dev_err(chip->dev, "Failed to initialize 88pm800
> devices\n");
>                 goto err_device_init;
>         }
> 
>         if (pdata && pdata->plat_config)
>                 pdata->plat_config(chip, pdata);

  this plat_config() is used in legacy non-device-tree code, it's used
  to implement fixup for chip or board level, it exists in
  the board configuration file

  just curious, do you think we still need to keep it?
  considering device-tree has been used. thanks;

> 
> Thanks,
> Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Yi Zhang]

On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> >On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>
> >>>>Add DT support to the 88pm800 driver, along with compatible
> >>>>field for it's sub-devices (rtc, onkey and regulator)
> >>>>
> >>>>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>---
> >>>>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>>>  1 file changed, 23 insertions(+)
> >>>>
> >>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>index 841717a..40fd014 100644
> >>>>--- a/drivers/mfd/88pm800.c
> >>>>+++ b/drivers/mfd/88pm800.c
> >>>>@@ -27,6 +27,7 @@
> >>>>  #include <linux/mfd/core.h>
> >>>>  #include <linux/mfd/88pm80x.h>
> >>>>  #include <linux/slab.h>
> >>>>+#include <linux/of_device.h>
> >>>>
> >>>>  /* Interrupt Registers */
> >>>>  #define PM800_INT_STATUS1		(0x05)
> >>>>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>>>  };
> >>>>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>>>
> >>>>+static const struct of_device_id pm80x_of_match_table[] = {
> >>>>+	{ .compatible = "marvell,88pm800", },
> >>>>+	{},
> >>>>+};
> >>>>+
> >>>>  static struct resource rtc_resources[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-rtc",
> >>>>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>>>  static struct mfd_cell rtc_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-rtc",
> >>>>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>>>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>>>  	 .resources = &rtc_resources[0],
> >>>>  	 .id = -1,
> >>>>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>>>  static const struct mfd_cell onkey_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-onkey",
> >>>>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>>>  	 .num_resources = 1,
> >>>>  	 .resources = &onkey_resources[0],
> >>>>  	 .id = -1,
> >>>>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>>>  static const struct mfd_cell regulator_devs[] = {
> >>>>  	{
> >>>>  	 .name = "88pm80x-regulator",
> >>>>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>>>  	 .id = -1,
> >>>>  	},
> >>>>  };
> >>>>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>>>  	int ret = 0;
> >>>>  	struct pm80x_chip *chip;
> >>>>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>>>+	struct device_node *np = client->dev.of_node;
> >>>>  	struct pm80x_subchip *subchip;
> >>>>
> >>>>+	if (!pdata && !np) {
> >>>>+		dev_err(&client->dev,
> >>>>+			"pm80x requires platform data or of_node\n");
> >>>>+		return -EINVAL;
> >>>>+	}
> >>>>+
> >>>>+	if (!pdata) {
> >>>>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>>>+		if (!pdata)
> >>>>+			return -ENOMEM;
> >>>>+	}
> >>>
> >>>Why have you allocated data for pdata, then done nothing with it?
> >>>
> >>
> >>Not in this patch, but subsequent patches would use it.
> >
> >Only provide it when you start using it please.
> >
> 
> I will take back my earlier comment of "not using in this patch, but
> subsequent patches".
> 
> pdata is being used, couple of places in the driver,
> 
> 
> @line-751
> 
>         ret = device_800_init(chip, pdata);
>         if (ret) {
>                 dev_err(chip->dev, "Failed to initialize 88pm800
> devices\n");
>                 goto err_device_init;
>         }
> 
>         if (pdata && pdata->plat_config)
>                 pdata->plat_config(chip, pdata);

  this plat_config() is used in legacy non-device-tree code, it's used
  to implement fixup for chip or board level, it exists in
  the board configuration file

  just curious, do you think we still need to keep it?
  considering device-tree has been used. thanks;

> 
> Thanks,
> Vaibhav

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
> On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
>>
>>
>> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
>>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>>> Add DT support to the 88pm800 driver, along with compatible
>>>>>> field for it's sub-devices (rtc, onkey and regulator)
>>>>>>
>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>> ---
>>>>>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>>>>>   1 file changed, 23 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>> index 841717a..40fd014 100644
>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>> @@ -27,6 +27,7 @@
>>>>>>   #include <linux/mfd/core.h>
>>>>>>   #include <linux/mfd/88pm80x.h>
>>>>>>   #include <linux/slab.h>
>>>>>> +#include <linux/of_device.h>
>>>>>>
>>>>>>   /* Interrupt Registers */
>>>>>>   #define PM800_INT_STATUS1		(0x05)
>>>>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>>>>>>   };
>>>>>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>>>>
>>>>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>>>>> +	{ .compatible = "marvell,88pm800", },
>>>>>> +	{},
>>>>>> +};
>>>>>> +
>>>>>>   static struct resource rtc_resources[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-rtc",
>>>>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>>>>>   static struct mfd_cell rtc_devs[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-rtc",
>>>>>> +	 .of_compatible = "marvell,88pm80x-rtc",
>>>>>>   	 .num_resources = ARRAY_SIZE(rtc_resources),
>>>>>>   	 .resources = &rtc_resources[0],
>>>>>>   	 .id = -1,
>>>>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>>>>>   static const struct mfd_cell onkey_devs[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-onkey",
>>>>>> +	 .of_compatible = "marvell,88pm80x-onkey",
>>>>>>   	 .num_resources = 1,
>>>>>>   	 .resources = &onkey_resources[0],
>>>>>>   	 .id = -1,
>>>>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>>>>>   static const struct mfd_cell regulator_devs[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-regulator",
>>>>>> +	 .of_compatible = "marvell,88pm80x-regulator",
>>>>>>   	 .id = -1,
>>>>>>   	},
>>>>>>   };
>>>>>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>>>>>>   	int ret = 0;
>>>>>>   	struct pm80x_chip *chip;
>>>>>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>>>>>> +	struct device_node *np = client->dev.of_node;
>>>>>>   	struct pm80x_subchip *subchip;
>>>>>>
>>>>>> +	if (!pdata && !np) {
>>>>>> +		dev_err(&client->dev,
>>>>>> +			"pm80x requires platform data or of_node\n");
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!pdata) {
>>>>>> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>>>>> +		if (!pdata)
>>>>>> +			return -ENOMEM;
>>>>>> +	}
>>>>>
>>>>> Why have you allocated data for pdata, then done nothing with it?
>>>>>
>>>>
>>>> Not in this patch, but subsequent patches would use it.
>>>
>>> Only provide it when you start using it please.
>>>
>>
>> I will take back my earlier comment of "not using in this patch, but
>> subsequent patches".
>>
>> pdata is being used, couple of places in the driver,
>>
>>
>> @line-751
>>
>>          ret = device_800_init(chip, pdata);
>>          if (ret) {
>>                  dev_err(chip->dev, "Failed to initialize 88pm800
>> devices\n");
>>                  goto err_device_init;
>>          }
>>
>>          if (pdata && pdata->plat_config)
>>                  pdata->plat_config(chip, pdata);
>
>    this plat_config() is used in legacy non-device-tree code, it's used
>    to implement fixup for chip or board level, it exists in
>    the board configuration file
>
>    just curious, do you think we still need to keep it?
>    considering device-tree has been used. thanks;
>

I do not see it anywhere in mainline kernel tree, is it part of some
internal tree?

If we know that it is being used, then lets not remove it now.

Thanks,
Vaibhav

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Vaibhav Hiremath]

On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
> On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
>>
>>
>> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
>>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>>> Add DT support to the 88pm800 driver, along with compatible
>>>>>> field for it's sub-devices (rtc, onkey and regulator)
>>>>>>
>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>> ---
>>>>>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>>>>>   1 file changed, 23 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>> index 841717a..40fd014 100644
>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>> @@ -27,6 +27,7 @@
>>>>>>   #include <linux/mfd/core.h>
>>>>>>   #include <linux/mfd/88pm80x.h>
>>>>>>   #include <linux/slab.h>
>>>>>> +#include <linux/of_device.h>
>>>>>>
>>>>>>   /* Interrupt Registers */
>>>>>>   #define PM800_INT_STATUS1		(0x05)
>>>>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
>>>>>>   };
>>>>>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>>>>
>>>>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>>>>> +	{ .compatible = "marvell,88pm800", },
>>>>>> +	{},
>>>>>> +};
>>>>>> +
>>>>>>   static struct resource rtc_resources[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-rtc",
>>>>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>>>>>   static struct mfd_cell rtc_devs[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-rtc",
>>>>>> +	 .of_compatible = "marvell,88pm80x-rtc",
>>>>>>   	 .num_resources = ARRAY_SIZE(rtc_resources),
>>>>>>   	 .resources = &rtc_resources[0],
>>>>>>   	 .id = -1,
>>>>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>>>>>   static const struct mfd_cell onkey_devs[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-onkey",
>>>>>> +	 .of_compatible = "marvell,88pm80x-onkey",
>>>>>>   	 .num_resources = 1,
>>>>>>   	 .resources = &onkey_resources[0],
>>>>>>   	 .id = -1,
>>>>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>>>>>   static const struct mfd_cell regulator_devs[] = {
>>>>>>   	{
>>>>>>   	 .name = "88pm80x-regulator",
>>>>>> +	 .of_compatible = "marvell,88pm80x-regulator",
>>>>>>   	 .id = -1,
>>>>>>   	},
>>>>>>   };
>>>>>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
>>>>>>   	int ret = 0;
>>>>>>   	struct pm80x_chip *chip;
>>>>>>   	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
>>>>>> +	struct device_node *np = client->dev.of_node;
>>>>>>   	struct pm80x_subchip *subchip;
>>>>>>
>>>>>> +	if (!pdata && !np) {
>>>>>> +		dev_err(&client->dev,
>>>>>> +			"pm80x requires platform data or of_node\n");
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!pdata) {
>>>>>> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>>>>> +		if (!pdata)
>>>>>> +			return -ENOMEM;
>>>>>> +	}
>>>>>
>>>>> Why have you allocated data for pdata, then done nothing with it?
>>>>>
>>>>
>>>> Not in this patch, but subsequent patches would use it.
>>>
>>> Only provide it when you start using it please.
>>>
>>
>> I will take back my earlier comment of "not using in this patch, but
>> subsequent patches".
>>
>> pdata is being used, couple of places in the driver,
>>
>>
>> @line-751
>>
>>          ret = device_800_init(chip, pdata);
>>          if (ret) {
>>                  dev_err(chip->dev, "Failed to initialize 88pm800
>> devices\n");
>>                  goto err_device_init;
>>          }
>>
>>          if (pdata && pdata->plat_config)
>>                  pdata->plat_config(chip, pdata);
>
>    this plat_config() is used in legacy non-device-tree code, it's used
>    to implement fixup for chip or board level, it exists in
>    the board configuration file
>
>    just curious, do you think we still need to keep it?
>    considering device-tree has been used. thanks;
>

I do not see it anywhere in mainline kernel tree, is it part of some
internal tree?

If we know that it is being used, then lets not remove it now.

Thanks,
Vaibhav

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Yi Zhang]

On Thu, Jun 25, 2015 at 03:26:29PM +0800, Vaibhav Hiremath wrote:
> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>  1 file changed, 54 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..f56b751
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,54 @@
> +* Marvell 88PM8xx Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm8xx chip
> +- interrupts : IRQ line for the 88pm8xx chip
> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm8xx local IRQ number
> +
> +88pm8xx family of devices consists of varied group of sub-devices:
  what about using "88pm80x family of devices..."? 
  for we also have another series of chip, with the name of 88pm88x..
> +
> +Device		 	Supply Names	 Description
> +------		 	------------	 -----------
> +88pm80x-onkey		:		: On key
> +88pm80x-rtc		:		: RTC
> +88pm80x-regulator	:		: Regulators
> +
> +Note: More device list will follow
> +
> +Example:
> +
> +	pmic: 88pm800@30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		regulators {
> +			compatible = "marvell,88pm80x-regulator";
> +
> +			buck1a: BUCK1A {
> +				regulator-name = "BUCK1A";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1: LDO1 {
> +				regulator-name = "LDO1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +
> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};
> -- 
> 1.9.1
> 

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Yi Zhang]

On Thu, Jun 25, 2015 at 03:26:29PM +0800, Vaibhav Hiremath wrote:
> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>  1 file changed, 54 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..f56b751
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,54 @@
> +* Marvell 88PM8xx Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm8xx chip
> +- interrupts : IRQ line for the 88pm8xx chip
> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm8xx local IRQ number
> +
> +88pm8xx family of devices consists of varied group of sub-devices:
  what about using "88pm80x family of devices..."? 
  for we also have another series of chip, with the name of 88pm88x..
> +
> +Device		 	Supply Names	 Description
> +------		 	------------	 -----------
> +88pm80x-onkey		:		: On key
> +88pm80x-rtc		:		: RTC
> +88pm80x-regulator	:		: Regulators
> +
> +Note: More device list will follow
> +
> +Example:
> +
> +	pmic: 88pm800@30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		regulators {
> +			compatible = "marvell,88pm80x-regulator";
> +
> +			buck1a: BUCK1A {
> +				regulator-name = "BUCK1A";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1: LDO1 {
> +				regulator-name = "LDO1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +
> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};
> -- 
> 1.9.1
> 

[PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Yi Zhang]

On Thu, Jun 25, 2015 at 03:26:29PM +0800, Vaibhav Hiremath wrote:
> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>  1 file changed, 54 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..f56b751
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> @@ -0,0 +1,54 @@
> +* Marvell 88PM8xx Power Management IC
> +
> +Required parent device properties:
> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
> +- reg : the I2C slave address for the 88pm8xx chip
> +- interrupts : IRQ line for the 88pm8xx chip
> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
> +- #interrupt-cells : should be 1.
> +		- The cell is the 88pm8xx local IRQ number
> +
> +88pm8xx family of devices consists of varied group of sub-devices:
  what about using "88pm80x family of devices..."? 
  for we also have another series of chip, with the name of 88pm88x..
> +
> +Device		 	Supply Names	 Description
> +------		 	------------	 -----------
> +88pm80x-onkey		:		: On key
> +88pm80x-rtc		:		: RTC
> +88pm80x-regulator	:		: Regulators
> +
> +Note: More device list will follow
> +
> +Example:
> +
> +	pmic: 88pm800 at 30 {
> +		compatible = "marvell,88pm800";
> +		reg = <0x30>;
> +		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		regulators {
> +			compatible = "marvell,88pm80x-regulator";
> +
> +			buck1a: BUCK1A {
> +				regulator-name = "BUCK1A";
> +				regulator-min-microvolt = <600000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1: LDO1 {
> +				regulator-name = "LDO1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +
> +		rtc {
> +			compatible = "marvell,88pm80x-rtc";
> +		};
> +	};
> -- 
> 1.9.1
> 

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Friday 26 June 2015 11:35 AM, Yi Zhang wrote:
> On Thu, Jun 25, 2015 at 03:26:29PM +0800, Vaibhav Hiremath wrote:
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>>   1 file changed, 54 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..f56b751
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,54 @@
>> +* Marvell 88PM8xx Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
>> +- reg : the I2C slave address for the 88pm8xx chip
>> +- interrupts : IRQ line for the 88pm8xx chip
>> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
>> +- #interrupt-cells : should be 1.
>> +		- The cell is the 88pm8xx local IRQ number
>> +
>> +88pm8xx family of devices consists of varied group of sub-devices:
>    what about using "88pm80x family of devices..."?
>    for we also have another series of chip, with the name of 88pm88x..

I renamed it after Rob's suggestion, but at that time we didn't see
88pm88x coming.
But I think now, I should stick to 88pm800 only, as it doesn't make
match with both 88pm822 and 88pm860.

Rob, let me know if you think otherwise.

Thanks,
Vaibhav

Re: [PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Friday 26 June 2015 11:35 AM, Yi Zhang wrote:
> On Thu, Jun 25, 2015 at 03:26:29PM +0800, Vaibhav Hiremath wrote:
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>>   1 file changed, 54 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..f56b751
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,54 @@
>> +* Marvell 88PM8xx Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
>> +- reg : the I2C slave address for the 88pm8xx chip
>> +- interrupts : IRQ line for the 88pm8xx chip
>> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
>> +- #interrupt-cells : should be 1.
>> +		- The cell is the 88pm8xx local IRQ number
>> +
>> +88pm8xx family of devices consists of varied group of sub-devices:
>    what about using "88pm80x family of devices..."?
>    for we also have another series of chip, with the name of 88pm88x..

I renamed it after Rob's suggestion, but at that time we didn't see
88pm88x coming.
But I think now, I should stick to 88pm800 only, as it doesn't make
match with both 88pm822 and 88pm860.

Rob, let me know if you think otherwise.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 3/3] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Friday 26 June 2015 11:35 AM, Yi Zhang wrote:
> On Thu, Jun 25, 2015 at 03:26:29PM +0800, Vaibhav Hiremath wrote:
>> With addition of DT support to 88pm800 mfd driver, this patch
>> adds new DT binding documentation along with respective properties.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 54 +++++++++++++++++++++++
>>   1 file changed, 54 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..f56b751
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
>> @@ -0,0 +1,54 @@
>> +* Marvell 88PM8xx Power Management IC
>> +
>> +Required parent device properties:
>> +- compatible : "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
>> +- reg : the I2C slave address for the 88pm8xx chip
>> +- interrupts : IRQ line for the 88pm8xx chip
>> +- interrupt-controller: describes the 88pm8xx as an interrupt controller
>> +- #interrupt-cells : should be 1.
>> +		- The cell is the 88pm8xx local IRQ number
>> +
>> +88pm8xx family of devices consists of varied group of sub-devices:
>    what about using "88pm80x family of devices..."?
>    for we also have another series of chip, with the name of 88pm88x..

I renamed it after Rob's suggestion, but at that time we didn't see
88pm88x coming.
But I think now, I should stick to 88pm800 only, as it doesn't make
match with both 88pm822 and 88pm860.

Rob, let me know if you think otherwise.

Thanks,
Vaibhav

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Krzysztof Kozlowski]

2015-06-26 14:59 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>
>
> On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
>>
>> On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
>>>
>>>
>>>
>>> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
>>>>
>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
>>>>>>
>>>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>>
>>>>>>> Add DT support to the 88pm800 driver, along with compatible
>>>>>>> field for it's sub-devices (rtc, onkey and regulator)
>>>>>>>
>>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>>> ---
>>>>>>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>>>>>>   1 file changed, 23 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>>> index 841717a..40fd014 100644
>>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>   #include <linux/mfd/core.h>
>>>>>>>   #include <linux/mfd/88pm80x.h>
>>>>>>>   #include <linux/slab.h>
>>>>>>> +#include <linux/of_device.h>
>>>>>>>
>>>>>>>   /* Interrupt Registers */
>>>>>>>   #define PM800_INT_STATUS1             (0x05)
>>>>>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id
>>>>>>> pm80x_id_table[] = {
>>>>>>>   };
>>>>>>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>>>>>
>>>>>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>>>>>> +       { .compatible = "marvell,88pm800", },
>>>>>>> +       {},
>>>>>>> +};
>>>>>>> +
>>>>>>>   static struct resource rtc_resources[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-rtc",
>>>>>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>>>>>>   static struct mfd_cell rtc_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-rtc",
>>>>>>> +        .of_compatible = "marvell,88pm80x-rtc",
>>>>>>>          .num_resources = ARRAY_SIZE(rtc_resources),
>>>>>>>          .resources = &rtc_resources[0],
>>>>>>>          .id = -1,
>>>>>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>>>>>>   static const struct mfd_cell onkey_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-onkey",
>>>>>>> +        .of_compatible = "marvell,88pm80x-onkey",
>>>>>>>          .num_resources = 1,
>>>>>>>          .resources = &onkey_resources[0],
>>>>>>>          .id = -1,
>>>>>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>>>>>>   static const struct mfd_cell regulator_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-regulator",
>>>>>>> +        .of_compatible = "marvell,88pm80x-regulator",
>>>>>>>          .id = -1,
>>>>>>>         },
>>>>>>>   };
>>>>>>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client
>>>>>>> *client,
>>>>>>>         int ret = 0;
>>>>>>>         struct pm80x_chip *chip;
>>>>>>>         struct pm80x_platform_data *pdata =
>>>>>>> dev_get_platdata(&client->dev);
>>>>>>> +       struct device_node *np = client->dev.of_node;
>>>>>>>         struct pm80x_subchip *subchip;
>>>>>>>
>>>>>>> +       if (!pdata && !np) {
>>>>>>> +               dev_err(&client->dev,
>>>>>>> +                       "pm80x requires platform data or of_node\n");
>>>>>>> +               return -EINVAL;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       if (!pdata) {
>>>>>>> +               pdata = devm_kzalloc(&client->dev, sizeof(*pdata),
>>>>>>> GFP_KERNEL);
>>>>>>> +               if (!pdata)
>>>>>>> +                       return -ENOMEM;
>>>>>>> +       }
>>>>>>
>>>>>>
>>>>>> Why have you allocated data for pdata, then done nothing with it?
>>>>>>
>>>>>
>>>>> Not in this patch, but subsequent patches would use it.
>>>>
>>>>
>>>> Only provide it when you start using it please.
>>>>
>>>
>>> I will take back my earlier comment of "not using in this patch, but
>>> subsequent patches".
>>>
>>> pdata is being used, couple of places in the driver,
>>>
>>>
>>> @line-751
>>>
>>>          ret = device_800_init(chip, pdata);
>>>          if (ret) {
>>>                  dev_err(chip->dev, "Failed to initialize 88pm800
>>> devices\n");
>>>                  goto err_device_init;
>>>          }
>>>
>>>          if (pdata && pdata->plat_config)
>>>                  pdata->plat_config(chip, pdata);
>>
>>
>>    this plat_config() is used in legacy non-device-tree code, it's used
>>    to implement fixup for chip or board level, it exists in
>>    the board configuration file
>>
>>    just curious, do you think we still need to keep it?
>>    considering device-tree has been used. thanks;
>>
>
> I do not see it anywhere in mainline kernel tree, is it part of some
> internal tree?
>
> If we know that it is being used, then lets not remove it now.

There aren't many board files in mainline but still downstream may use
it. Do you know who could be the downstream for this device?

Best regards,
Krzysztof

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Krzysztof Kozlowski]

2015-06-26 14:59 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
>
>
> On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
>>
>> On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
>>>
>>>
>>>
>>> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
>>>>
>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
>>>>>>
>>>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>>
>>>>>>> Add DT support to the 88pm800 driver, along with compatible
>>>>>>> field for it's sub-devices (rtc, onkey and regulator)
>>>>>>>
>>>>>>> Signed-off-by: Chao Xie <chao.xie-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>> ---
>>>>>>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>>>>>>   1 file changed, 23 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>>> index 841717a..40fd014 100644
>>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>   #include <linux/mfd/core.h>
>>>>>>>   #include <linux/mfd/88pm80x.h>
>>>>>>>   #include <linux/slab.h>
>>>>>>> +#include <linux/of_device.h>
>>>>>>>
>>>>>>>   /* Interrupt Registers */
>>>>>>>   #define PM800_INT_STATUS1             (0x05)
>>>>>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id
>>>>>>> pm80x_id_table[] = {
>>>>>>>   };
>>>>>>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>>>>>
>>>>>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>>>>>> +       { .compatible = "marvell,88pm800", },
>>>>>>> +       {},
>>>>>>> +};
>>>>>>> +
>>>>>>>   static struct resource rtc_resources[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-rtc",
>>>>>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>>>>>>   static struct mfd_cell rtc_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-rtc",
>>>>>>> +        .of_compatible = "marvell,88pm80x-rtc",
>>>>>>>          .num_resources = ARRAY_SIZE(rtc_resources),
>>>>>>>          .resources = &rtc_resources[0],
>>>>>>>          .id = -1,
>>>>>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>>>>>>   static const struct mfd_cell onkey_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-onkey",
>>>>>>> +        .of_compatible = "marvell,88pm80x-onkey",
>>>>>>>          .num_resources = 1,
>>>>>>>          .resources = &onkey_resources[0],
>>>>>>>          .id = -1,
>>>>>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>>>>>>   static const struct mfd_cell regulator_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-regulator",
>>>>>>> +        .of_compatible = "marvell,88pm80x-regulator",
>>>>>>>          .id = -1,
>>>>>>>         },
>>>>>>>   };
>>>>>>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client
>>>>>>> *client,
>>>>>>>         int ret = 0;
>>>>>>>         struct pm80x_chip *chip;
>>>>>>>         struct pm80x_platform_data *pdata =
>>>>>>> dev_get_platdata(&client->dev);
>>>>>>> +       struct device_node *np = client->dev.of_node;
>>>>>>>         struct pm80x_subchip *subchip;
>>>>>>>
>>>>>>> +       if (!pdata && !np) {
>>>>>>> +               dev_err(&client->dev,
>>>>>>> +                       "pm80x requires platform data or of_node\n");
>>>>>>> +               return -EINVAL;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       if (!pdata) {
>>>>>>> +               pdata = devm_kzalloc(&client->dev, sizeof(*pdata),
>>>>>>> GFP_KERNEL);
>>>>>>> +               if (!pdata)
>>>>>>> +                       return -ENOMEM;
>>>>>>> +       }
>>>>>>
>>>>>>
>>>>>> Why have you allocated data for pdata, then done nothing with it?
>>>>>>
>>>>>
>>>>> Not in this patch, but subsequent patches would use it.
>>>>
>>>>
>>>> Only provide it when you start using it please.
>>>>
>>>
>>> I will take back my earlier comment of "not using in this patch, but
>>> subsequent patches".
>>>
>>> pdata is being used, couple of places in the driver,
>>>
>>>
>>> @line-751
>>>
>>>          ret = device_800_init(chip, pdata);
>>>          if (ret) {
>>>                  dev_err(chip->dev, "Failed to initialize 88pm800
>>> devices\n");
>>>                  goto err_device_init;
>>>          }
>>>
>>>          if (pdata && pdata->plat_config)
>>>                  pdata->plat_config(chip, pdata);
>>
>>
>>    this plat_config() is used in legacy non-device-tree code, it's used
>>    to implement fixup for chip or board level, it exists in
>>    the board configuration file
>>
>>    just curious, do you think we still need to keep it?
>>    considering device-tree has been used. thanks;
>>
>
> I do not see it anywhere in mainline kernel tree, is it part of some
> internal tree?
>
> If we know that it is being used, then lets not remove it now.

There aren't many board files in mainline but still downstream may use
it. Do you know who could be the downstream for this device?

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Krzysztof Kozlowski]

2015-06-26 14:59 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>
>
> On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
>>
>> On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
>>>
>>>
>>>
>>> On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
>>>>
>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>> On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
>>>>>>
>>>>>> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>>>>>>
>>>>>>> Add DT support to the 88pm800 driver, along with compatible
>>>>>>> field for it's sub-devices (rtc, onkey and regulator)
>>>>>>>
>>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>>> ---
>>>>>>>   drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>>>>>>>   1 file changed, 23 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>>> index 841717a..40fd014 100644
>>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>   #include <linux/mfd/core.h>
>>>>>>>   #include <linux/mfd/88pm80x.h>
>>>>>>>   #include <linux/slab.h>
>>>>>>> +#include <linux/of_device.h>
>>>>>>>
>>>>>>>   /* Interrupt Registers */
>>>>>>>   #define PM800_INT_STATUS1             (0x05)
>>>>>>> @@ -121,6 +122,11 @@ static const struct i2c_device_id
>>>>>>> pm80x_id_table[] = {
>>>>>>>   };
>>>>>>>   MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
>>>>>>>
>>>>>>> +static const struct of_device_id pm80x_of_match_table[] = {
>>>>>>> +       { .compatible = "marvell,88pm800", },
>>>>>>> +       {},
>>>>>>> +};
>>>>>>> +
>>>>>>>   static struct resource rtc_resources[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-rtc",
>>>>>>> @@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
>>>>>>>   static struct mfd_cell rtc_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-rtc",
>>>>>>> +        .of_compatible = "marvell,88pm80x-rtc",
>>>>>>>          .num_resources = ARRAY_SIZE(rtc_resources),
>>>>>>>          .resources = &rtc_resources[0],
>>>>>>>          .id = -1,
>>>>>>> @@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
>>>>>>>   static const struct mfd_cell onkey_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-onkey",
>>>>>>> +        .of_compatible = "marvell,88pm80x-onkey",
>>>>>>>          .num_resources = 1,
>>>>>>>          .resources = &onkey_resources[0],
>>>>>>>          .id = -1,
>>>>>>> @@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
>>>>>>>   static const struct mfd_cell regulator_devs[] = {
>>>>>>>         {
>>>>>>>          .name = "88pm80x-regulator",
>>>>>>> +        .of_compatible = "marvell,88pm80x-regulator",
>>>>>>>          .id = -1,
>>>>>>>         },
>>>>>>>   };
>>>>>>> @@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client
>>>>>>> *client,
>>>>>>>         int ret = 0;
>>>>>>>         struct pm80x_chip *chip;
>>>>>>>         struct pm80x_platform_data *pdata =
>>>>>>> dev_get_platdata(&client->dev);
>>>>>>> +       struct device_node *np = client->dev.of_node;
>>>>>>>         struct pm80x_subchip *subchip;
>>>>>>>
>>>>>>> +       if (!pdata && !np) {
>>>>>>> +               dev_err(&client->dev,
>>>>>>> +                       "pm80x requires platform data or of_node\n");
>>>>>>> +               return -EINVAL;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       if (!pdata) {
>>>>>>> +               pdata = devm_kzalloc(&client->dev, sizeof(*pdata),
>>>>>>> GFP_KERNEL);
>>>>>>> +               if (!pdata)
>>>>>>> +                       return -ENOMEM;
>>>>>>> +       }
>>>>>>
>>>>>>
>>>>>> Why have you allocated data for pdata, then done nothing with it?
>>>>>>
>>>>>
>>>>> Not in this patch, but subsequent patches would use it.
>>>>
>>>>
>>>> Only provide it when you start using it please.
>>>>
>>>
>>> I will take back my earlier comment of "not using in this patch, but
>>> subsequent patches".
>>>
>>> pdata is being used, couple of places in the driver,
>>>
>>>
>>> @line-751
>>>
>>>          ret = device_800_init(chip, pdata);
>>>          if (ret) {
>>>                  dev_err(chip->dev, "Failed to initialize 88pm800
>>> devices\n");
>>>                  goto err_device_init;
>>>          }
>>>
>>>          if (pdata && pdata->plat_config)
>>>                  pdata->plat_config(chip, pdata);
>>
>>
>>    this plat_config() is used in legacy non-device-tree code, it's used
>>    to implement fixup for chip or board level, it exists in
>>    the board configuration file
>>
>>    just curious, do you think we still need to keep it?
>>    considering device-tree has been used. thanks;
>>
>
> I do not see it anywhere in mainline kernel tree, is it part of some
> internal tree?
>
> If we know that it is being used, then lets not remove it now.

There aren't many board files in mainline but still downstream may use
it. Do you know who could be the downstream for this device?

Best regards,
Krzysztof

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Yi Zhang]

On Fri, Jun 26, 2015 at 11:29:29AM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
> >On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
> >>
> >>
> >>On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> >>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>>On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >>>>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>>>
> >>>>>>Add DT support to the 88pm800 driver, along with compatible
> >>>>>>field for it's sub-devices (rtc, onkey and regulator)
> >>>>>>
> >>>>>>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>>>---
> >>>>>>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>>>>>  1 file changed, 23 insertions(+)
> >>>>>>
> >>>>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>>>index 841717a..40fd014 100644
> >>>>>>--- a/drivers/mfd/88pm800.c
> >>>>>>+++ b/drivers/mfd/88pm800.c
> >>>>>>@@ -27,6 +27,7 @@
> >>>>>>  #include <linux/mfd/core.h>
> >>>>>>  #include <linux/mfd/88pm80x.h>
> >>>>>>  #include <linux/slab.h>
> >>>>>>+#include <linux/of_device.h>
> >>>>>>
> >>>>>>  /* Interrupt Registers */
> >>>>>>  #define PM800_INT_STATUS1		(0x05)
> >>>>>>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>>>>>  };
> >>>>>>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>>>>>
> >>>>>>+static const struct of_device_id pm80x_of_match_table[] = {
> >>>>>>+	{ .compatible = "marvell,88pm800", },
> >>>>>>+	{},
> >>>>>>+};
> >>>>>>+
> >>>>>>  static struct resource rtc_resources[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-rtc",
> >>>>>>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>>>>>  static struct mfd_cell rtc_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-rtc",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>>>>>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>>>>>  	 .resources = &rtc_resources[0],
> >>>>>>  	 .id = -1,
> >>>>>>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>>>>>  static const struct mfd_cell onkey_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-onkey",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>>>>>  	 .num_resources = 1,
> >>>>>>  	 .resources = &onkey_resources[0],
> >>>>>>  	 .id = -1,
> >>>>>>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>>>>>  static const struct mfd_cell regulator_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-regulator",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>>>>>  	 .id = -1,
> >>>>>>  	},
> >>>>>>  };
> >>>>>>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>>>>>  	int ret = 0;
> >>>>>>  	struct pm80x_chip *chip;
> >>>>>>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>>>>>+	struct device_node *np = client->dev.of_node;
> >>>>>>  	struct pm80x_subchip *subchip;
> >>>>>>
> >>>>>>+	if (!pdata && !np) {
> >>>>>>+		dev_err(&client->dev,
> >>>>>>+			"pm80x requires platform data or of_node\n");
> >>>>>>+		return -EINVAL;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	if (!pdata) {
> >>>>>>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>>>>>+		if (!pdata)
> >>>>>>+			return -ENOMEM;
> >>>>>>+	}
> >>>>>
> >>>>>Why have you allocated data for pdata, then done nothing with it?
> >>>>>
> >>>>
> >>>>Not in this patch, but subsequent patches would use it.
> >>>
> >>>Only provide it when you start using it please.
> >>>
> >>
> >>I will take back my earlier comment of "not using in this patch, but
> >>subsequent patches".
> >>
> >>pdata is being used, couple of places in the driver,
> >>
> >>
> >>@line-751
> >>
> >>         ret = device_800_init(chip, pdata);
> >>         if (ret) {
> >>                 dev_err(chip->dev, "Failed to initialize 88pm800
> >>devices\n");
> >>                 goto err_device_init;
> >>         }
> >>
> >>         if (pdata && pdata->plat_config)
> >>                 pdata->plat_config(chip, pdata);
> >
> >   this plat_config() is used in legacy non-device-tree code, it's used
> >   to implement fixup for chip or board level, it exists in
> >   the board configuration file
> >
> >   just curious, do you think we still need to keep it?
> >   considering device-tree has been used. thanks;
> >
> 
> I do not see it anywhere in mainline kernel tree, is it part of some
> internal tree?
> 
> If we know that it is being used, then lets not remove it now.

  Yes, got your point, it may still be used by other trees, thanks;
> 
> Thanks,
> Vaibhav

Re: [PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Yi Zhang]

On Fri, Jun 26, 2015 at 11:29:29AM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
> >On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
> >>
> >>
> >>On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> >>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>>On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >>>>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>>>
> >>>>>>Add DT support to the 88pm800 driver, along with compatible
> >>>>>>field for it's sub-devices (rtc, onkey and regulator)
> >>>>>>
> >>>>>>Signed-off-by: Chao Xie <chao.xie-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >>>>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>>>---
> >>>>>>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>>>>>  1 file changed, 23 insertions(+)
> >>>>>>
> >>>>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>>>index 841717a..40fd014 100644
> >>>>>>--- a/drivers/mfd/88pm800.c
> >>>>>>+++ b/drivers/mfd/88pm800.c
> >>>>>>@@ -27,6 +27,7 @@
> >>>>>>  #include <linux/mfd/core.h>
> >>>>>>  #include <linux/mfd/88pm80x.h>
> >>>>>>  #include <linux/slab.h>
> >>>>>>+#include <linux/of_device.h>
> >>>>>>
> >>>>>>  /* Interrupt Registers */
> >>>>>>  #define PM800_INT_STATUS1		(0x05)
> >>>>>>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>>>>>  };
> >>>>>>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>>>>>
> >>>>>>+static const struct of_device_id pm80x_of_match_table[] = {
> >>>>>>+	{ .compatible = "marvell,88pm800", },
> >>>>>>+	{},
> >>>>>>+};
> >>>>>>+
> >>>>>>  static struct resource rtc_resources[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-rtc",
> >>>>>>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>>>>>  static struct mfd_cell rtc_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-rtc",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>>>>>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>>>>>  	 .resources = &rtc_resources[0],
> >>>>>>  	 .id = -1,
> >>>>>>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>>>>>  static const struct mfd_cell onkey_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-onkey",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>>>>>  	 .num_resources = 1,
> >>>>>>  	 .resources = &onkey_resources[0],
> >>>>>>  	 .id = -1,
> >>>>>>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>>>>>  static const struct mfd_cell regulator_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-regulator",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>>>>>  	 .id = -1,
> >>>>>>  	},
> >>>>>>  };
> >>>>>>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>>>>>  	int ret = 0;
> >>>>>>  	struct pm80x_chip *chip;
> >>>>>>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>>>>>+	struct device_node *np = client->dev.of_node;
> >>>>>>  	struct pm80x_subchip *subchip;
> >>>>>>
> >>>>>>+	if (!pdata && !np) {
> >>>>>>+		dev_err(&client->dev,
> >>>>>>+			"pm80x requires platform data or of_node\n");
> >>>>>>+		return -EINVAL;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	if (!pdata) {
> >>>>>>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>>>>>+		if (!pdata)
> >>>>>>+			return -ENOMEM;
> >>>>>>+	}
> >>>>>
> >>>>>Why have you allocated data for pdata, then done nothing with it?
> >>>>>
> >>>>
> >>>>Not in this patch, but subsequent patches would use it.
> >>>
> >>>Only provide it when you start using it please.
> >>>
> >>
> >>I will take back my earlier comment of "not using in this patch, but
> >>subsequent patches".
> >>
> >>pdata is being used, couple of places in the driver,
> >>
> >>
> >>@line-751
> >>
> >>         ret = device_800_init(chip, pdata);
> >>         if (ret) {
> >>                 dev_err(chip->dev, "Failed to initialize 88pm800
> >>devices\n");
> >>                 goto err_device_init;
> >>         }
> >>
> >>         if (pdata && pdata->plat_config)
> >>                 pdata->plat_config(chip, pdata);
> >
> >   this plat_config() is used in legacy non-device-tree code, it's used
> >   to implement fixup for chip or board level, it exists in
> >   the board configuration file
> >
> >   just curious, do you think we still need to keep it?
> >   considering device-tree has been used. thanks;
> >
> 
> I do not see it anywhere in mainline kernel tree, is it part of some
> internal tree?
> 
> If we know that it is being used, then lets not remove it now.

  Yes, got your point, it may still be used by other trees, thanks;
> 
> Thanks,
> Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 1/3] mfd: 88pm800: Add device tree support

[Yi Zhang]

On Fri, Jun 26, 2015 at 11:29:29AM +0530, Vaibhav Hiremath wrote:
> 
> 
> On Friday 26 June 2015 11:23 AM, Yi Zhang wrote:
> >On Thu, Jun 25, 2015 at 08:57:49PM +0530, Vaibhav Hiremath wrote:
> >>
> >>
> >>On Thursday 25 June 2015 08:18 PM, Lee Jones wrote:
> >>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>>On Thursday 25 June 2015 03:49 PM, Lee Jones wrote:
> >>>>>On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
> >>>>>
> >>>>>>Add DT support to the 88pm800 driver, along with compatible
> >>>>>>field for it's sub-devices (rtc, onkey and regulator)
> >>>>>>
> >>>>>>Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>>>---
> >>>>>>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
> >>>>>>  1 file changed, 23 insertions(+)
> >>>>>>
> >>>>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>>>index 841717a..40fd014 100644
> >>>>>>--- a/drivers/mfd/88pm800.c
> >>>>>>+++ b/drivers/mfd/88pm800.c
> >>>>>>@@ -27,6 +27,7 @@
> >>>>>>  #include <linux/mfd/core.h>
> >>>>>>  #include <linux/mfd/88pm80x.h>
> >>>>>>  #include <linux/slab.h>
> >>>>>>+#include <linux/of_device.h>
> >>>>>>
> >>>>>>  /* Interrupt Registers */
> >>>>>>  #define PM800_INT_STATUS1		(0x05)
> >>>>>>@@ -121,6 +122,11 @@ static const struct i2c_device_id pm80x_id_table[] = {
> >>>>>>  };
> >>>>>>  MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> >>>>>>
> >>>>>>+static const struct of_device_id pm80x_of_match_table[] = {
> >>>>>>+	{ .compatible = "marvell,88pm800", },
> >>>>>>+	{},
> >>>>>>+};
> >>>>>>+
> >>>>>>  static struct resource rtc_resources[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-rtc",
> >>>>>>@@ -133,6 +139,7 @@ static struct resource rtc_resources[] = {
> >>>>>>  static struct mfd_cell rtc_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-rtc",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-rtc",
> >>>>>>  	 .num_resources = ARRAY_SIZE(rtc_resources),
> >>>>>>  	 .resources = &rtc_resources[0],
> >>>>>>  	 .id = -1,
> >>>>>>@@ -151,6 +158,7 @@ static struct resource onkey_resources[] = {
> >>>>>>  static const struct mfd_cell onkey_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-onkey",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-onkey",
> >>>>>>  	 .num_resources = 1,
> >>>>>>  	 .resources = &onkey_resources[0],
> >>>>>>  	 .id = -1,
> >>>>>>@@ -160,6 +168,7 @@ static const struct mfd_cell onkey_devs[] = {
> >>>>>>  static const struct mfd_cell regulator_devs[] = {
> >>>>>>  	{
> >>>>>>  	 .name = "88pm80x-regulator",
> >>>>>>+	 .of_compatible = "marvell,88pm80x-regulator",
> >>>>>>  	 .id = -1,
> >>>>>>  	},
> >>>>>>  };
> >>>>>>@@ -544,8 +553,21 @@ static int pm800_probe(struct i2c_client *client,
> >>>>>>  	int ret = 0;
> >>>>>>  	struct pm80x_chip *chip;
> >>>>>>  	struct pm80x_platform_data *pdata = dev_get_platdata(&client->dev);
> >>>>>>+	struct device_node *np = client->dev.of_node;
> >>>>>>  	struct pm80x_subchip *subchip;
> >>>>>>
> >>>>>>+	if (!pdata && !np) {
> >>>>>>+		dev_err(&client->dev,
> >>>>>>+			"pm80x requires platform data or of_node\n");
> >>>>>>+		return -EINVAL;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	if (!pdata) {
> >>>>>>+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>>>>>+		if (!pdata)
> >>>>>>+			return -ENOMEM;
> >>>>>>+	}
> >>>>>
> >>>>>Why have you allocated data for pdata, then done nothing with it?
> >>>>>
> >>>>
> >>>>Not in this patch, but subsequent patches would use it.
> >>>
> >>>Only provide it when you start using it please.
> >>>
> >>
> >>I will take back my earlier comment of "not using in this patch, but
> >>subsequent patches".
> >>
> >>pdata is being used, couple of places in the driver,
> >>
> >>
> >>@line-751
> >>
> >>         ret = device_800_init(chip, pdata);
> >>         if (ret) {
> >>                 dev_err(chip->dev, "Failed to initialize 88pm800
> >>devices\n");
> >>                 goto err_device_init;
> >>         }
> >>
> >>         if (pdata && pdata->plat_config)
> >>                 pdata->plat_config(chip, pdata);
> >
> >   this plat_config() is used in legacy non-device-tree code, it's used
> >   to implement fixup for chip or board level, it exists in
> >   the board configuration file
> >
> >   just curious, do you think we still need to keep it?
> >   considering device-tree has been used. thanks;
> >
> 
> I do not see it anywhere in mainline kernel tree, is it part of some
> internal tree?
> 
> If we know that it is being used, then lets not remove it now.

  Yes, got your point, it may still be used by other trees, thanks;
> 
> Thanks,
> Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name.
>>
>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 40fd014..e0cd7ad 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)

<snip>

>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>>   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   		if (!pdata)
>>   			return -ENOMEM;
>> +
>> +		/* by default, set irq clear method on write */
>> +		pdata->irq_clr_on_wr = true;
>
> You can save yourself some memory here, by removing this seemingly
> pointless allocation and do this above:
>
>          chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;
>
>>   	}


I accepted quickly earlier, without giving second thought.

We need pdata, as it is being used in multiple places inside driver.
And I do not want to break that. So allocation of pdata is indeed
needed here.

And also, in order to put your suggested line of code is concerned, I
have to rearrange the code, as access to "chip" is only available after
pm80x_init()

And I feel that would look ugly.

So I will stick to my original code as far as this comment is concerned.


Note that, your comment on CLR_ON_WRITE and CLR_ON_READ already taken
care of.

Thanks,
Vaibhav

Re: [PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name.
>>
>> Signed-off-by: Zhao Ye <zhaoy-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 40fd014..e0cd7ad 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)

<snip>

>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>>   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   		if (!pdata)
>>   			return -ENOMEM;
>> +
>> +		/* by default, set irq clear method on write */
>> +		pdata->irq_clr_on_wr = true;
>
> You can save yourself some memory here, by removing this seemingly
> pointless allocation and do this above:
>
>          chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;
>
>>   	}


I accepted quickly earlier, without giving second thought.

We need pdata, as it is being used in multiple places inside driver.
And I do not want to break that. So allocation of pdata is indeed
needed here.

And also, in order to put your suggested line of code is concerned, I
have to rearrange the code, as access to "chip" is only available after
pm80x_init()

And I feel that would look ugly.

So I will stick to my original code as far as this comment is concerned.


Note that, your comment on CLR_ON_WRITE and CLR_ON_READ already taken
care of.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH-v4 2/3] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Thursday 25 June 2015 03:56 PM, Lee Jones wrote:
> On Thu, 25 Jun 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name.
>>
>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>   include/linux/mfd/88pm80x.h |  6 ++++--
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index 40fd014..e0cd7ad 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -376,7 +376,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)

<snip>

>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -566,6 +568,9 @@ static int pm800_probe(struct i2c_client *client,
>>   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   		if (!pdata)
>>   			return -ENOMEM;
>> +
>> +		/* by default, set irq clear method on write */
>> +		pdata->irq_clr_on_wr = true;
>
> You can save yourself some memory here, by removing this seemingly
> pointless allocation and do this above:
>
>          chip->irq_clr_on_wr = pdata ? pdata->irq_clr_on_wr : true;
>
>>   	}


I accepted quickly earlier, without giving second thought.

We need pdata, as it is being used in multiple places inside driver.
And I do not want to break that. So allocation of pdata is indeed
needed here.

And also, in order to put your suggested line of code is concerned, I
have to rearrange the code, as access to "chip" is only available after
pm80x_init()

And I feel that would look ugly.

So I will stick to my original code as far as this comment is concerned.


Note that, your comment on CLR_ON_WRITE and CLR_ON_READ already taken
care of.

Thanks,
Vaibhav