[PATCH-v6 1/6] mfd: 88pm800: remove duplicate dev_err calls during probe

[PATCH-v6 1/6] mfd: 88pm800: remove duplicate dev_err calls during probe

[Vaibhav Hiremath]

During probe, inside fn device_800_init(), submodule init is
happening and the same error message is getting printed in both
the places, <subdev>_init() and inside device_800_init().

So this patch gets rid of dev_err from <subdev>_init() fns.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mfd/88pm800.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 841717a..0c5470e 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -308,59 +308,35 @@ static int device_gpadc_init(struct pm80x_chip *chip,
 	return 0;
 
 out:
-	dev_info(chip->dev, "pm800 device_gpadc_init: Failed!\n");
 	return ret;
 }
 
 static int device_onkey_init(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata)
 {
-	int ret;
-
-	ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
+	return mfd_add_devices(chip->dev, 0, &onkey_devs[0],
 			      ARRAY_SIZE(onkey_devs), &onkey_resources[0], 0,
 			      NULL);
-	if (ret) {
-		dev_err(chip->dev, "Failed to add onkey subdev\n");
-		return ret;
-	}
-
-	return 0;
 }
 
 static int device_rtc_init(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata)
 {
-	int ret;
-
 	if (pdata) {
 		rtc_devs[0].platform_data = pdata->rtc;
 		rtc_devs[0].pdata_size =
 				pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
 	}
-	ret = mfd_add_devices(chip->dev, 0, &rtc_devs[0],
-			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
-	if (ret) {
-		dev_err(chip->dev, "Failed to add rtc subdev\n");
-		return ret;
-	}
 
-	return 0;
+	return mfd_add_devices(chip->dev, 0, &rtc_devs[0],
+			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
 }
 
 static int device_regulator_init(struct pm80x_chip *chip,
 					   struct pm80x_platform_data *pdata)
 {
-	int ret;
-
-	ret = mfd_add_devices(chip->dev, 0, &regulator_devs[0],
+	return mfd_add_devices(chip->dev, 0, &regulator_devs[0],
 			      ARRAY_SIZE(regulator_devs), NULL, 0, NULL);
-	if (ret) {
-		dev_err(chip->dev, "Failed to add regulator subdev\n");
-		return ret;
-	}
-
-	return 0;
 }
 
 static int device_irq_init_800(struct pm80x_chip *chip)
-- 
1.9.1

Re: [PATCH-v6 1/6] mfd: 88pm800: remove duplicate dev_err calls during probe

[Lee Jones]

On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:

> During probe, inside fn device_800_init(), submodule init is
> happening and the same error message is getting printed in both
> the places, <subdev>_init() and inside device_800_init().
> 
> So this patch gets rid of dev_err from <subdev>_init() fns.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 32 ++++----------------------------
>  1 file changed, 4 insertions(+), 28 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 841717a..0c5470e 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -308,59 +308,35 @@ static int device_gpadc_init(struct pm80x_chip *chip,
>  	return 0;
>  
>  out:
> -	dev_info(chip->dev, "pm800 device_gpadc_init: Failed!\n");
>  	return ret;
>  }
>  
>  static int device_onkey_init(struct pm80x_chip *chip,
>  				struct pm80x_platform_data *pdata)
>  {
> -	int ret;
> -
> -	ret = mfd_add_devices(chip->dev, 0, &onkey_devs[0],
> +	return mfd_add_devices(chip->dev, 0, &onkey_devs[0],
>  			      ARRAY_SIZE(onkey_devs), &onkey_resources[0], 0,
>  			      NULL);
> -	if (ret) {
> -		dev_err(chip->dev, "Failed to add onkey subdev\n");
> -		return ret;
> -	}
> -
> -	return 0;
>  }
>  
>  static int device_rtc_init(struct pm80x_chip *chip,
>  				struct pm80x_platform_data *pdata)
>  {
> -	int ret;
> -
>  	if (pdata) {
>  		rtc_devs[0].platform_data = pdata->rtc;
>  		rtc_devs[0].pdata_size =
>  				pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
>  	}
> -	ret = mfd_add_devices(chip->dev, 0, &rtc_devs[0],
> -			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
> -	if (ret) {
> -		dev_err(chip->dev, "Failed to add rtc subdev\n");
> -		return ret;
> -	}
>  
> -	return 0;
> +	return mfd_add_devices(chip->dev, 0, &rtc_devs[0],
> +			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
>  }
>  
>  static int device_regulator_init(struct pm80x_chip *chip,
>  					   struct pm80x_platform_data *pdata)
>  {
> -	int ret;
> -
> -	ret = mfd_add_devices(chip->dev, 0, &regulator_devs[0],
> +	return mfd_add_devices(chip->dev, 0, &regulator_devs[0],
>  			      ARRAY_SIZE(regulator_devs), NULL, 0, NULL);
> -	if (ret) {
> -		dev_err(chip->dev, "Failed to add regulator subdev\n");
> -		return ret;
> -	}
> -
> -	return 0;
>  }
>  
>  static int device_irq_init_800(struct pm80x_chip *chip)

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

[PATCH-v6 2/6] 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>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/88pm800.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 0c5470e..95c418c 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,
 	},
 };
@@ -520,8 +529,24 @@ 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;
+
+		/* Ensure we only alloc platform data once */
+		client->dev.platform_data = pdata;
+	}
+
 	ret = pm80x_init(client);
 	if (ret) {
 		dev_err(&client->dev, "pm800_init fail\n");
@@ -587,6 +612,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-v6 3/6] mfd: 88pm800: Get pdata from 'device' rather than passing as a parameter

[Vaibhav Hiremath]

Currently the device_xxx_init() fns take pdata as an input parameter to
the fn, but the cleaner approach would be to use dev_get_platdata() to
get the pdata.
So this patch changes the code accordingly.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mfd/88pm800.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 95c418c..af8232f 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -254,11 +254,11 @@ static const struct regmap_irq pm800_irqs[] = {
 	},
 };
 
-static int device_gpadc_init(struct pm80x_chip *chip,
-				       struct pm80x_platform_data *pdata)
+static int device_gpadc_init(struct pm80x_chip *chip)
 {
 	struct pm80x_subchip *subchip = chip->subchip;
 	struct regmap *map = subchip->regmap_gpadc;
+	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
 	int data = 0, mask = 0, ret = 0;
 
 	if (!map) {
@@ -320,17 +320,17 @@ out:
 	return ret;
 }
 
-static int device_onkey_init(struct pm80x_chip *chip,
-				struct pm80x_platform_data *pdata)
+static int device_onkey_init(struct pm80x_chip *chip)
 {
 	return mfd_add_devices(chip->dev, 0, &onkey_devs[0],
 			      ARRAY_SIZE(onkey_devs), &onkey_resources[0], 0,
 			      NULL);
 }
 
-static int device_rtc_init(struct pm80x_chip *chip,
-				struct pm80x_platform_data *pdata)
+static int device_rtc_init(struct pm80x_chip *chip)
 {
+	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
+
 	if (pdata) {
 		rtc_devs[0].platform_data = pdata->rtc;
 		rtc_devs[0].pdata_size =
@@ -341,8 +341,7 @@ static int device_rtc_init(struct pm80x_chip *chip,
 			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
 }
 
-static int device_regulator_init(struct pm80x_chip *chip,
-					   struct pm80x_platform_data *pdata)
+static int device_regulator_init(struct pm80x_chip *chip)
 {
 	return mfd_add_devices(chip->dev, 0, &regulator_devs[0],
 			      ARRAY_SIZE(regulator_devs), NULL, 0, NULL);
@@ -463,11 +462,11 @@ static void pm800_pages_exit(struct pm80x_chip *chip)
 		i2c_unregister_device(subchip->gpadc_page);
 }
 
-static int device_800_init(struct pm80x_chip *chip,
-				     struct pm80x_platform_data *pdata)
+static int device_800_init(struct pm80x_chip *chip)
 {
 	int ret;
 	unsigned int val;
+	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
 
 	/*
 	 * alarm wake up bit will be clear in device_irq_init(),
@@ -483,7 +482,7 @@ static int device_800_init(struct pm80x_chip *chip,
 			pdata->rtc->rtc_wakeup = 1;
 	}
 
-	ret = device_gpadc_init(chip, pdata);
+	ret = device_gpadc_init(chip);
 	if (ret < 0) {
 		dev_err(chip->dev, "[%s]Failed to init gpadc\n", __func__);
 		goto out;
@@ -497,19 +496,19 @@ static int device_800_init(struct pm80x_chip *chip,
 		goto out;
 	}
 
-	ret = device_onkey_init(chip, pdata);
+	ret = device_onkey_init(chip);
 	if (ret) {
 		dev_err(chip->dev, "Failed to add onkey subdev\n");
 		goto out_dev;
 	}
 
-	ret = device_rtc_init(chip, pdata);
+	ret = device_rtc_init(chip);
 	if (ret) {
 		dev_err(chip->dev, "Failed to add rtc subdev\n");
 		goto out;
 	}
 
-	ret = device_regulator_init(chip, pdata);
+	ret = device_regulator_init(chip);
 	if (ret) {
 		dev_err(chip->dev, "Failed to add regulators subdev\n");
 		goto out;
@@ -575,7 +574,7 @@ static int pm800_probe(struct i2c_client *client,
 		goto err_device_init;
 	}
 
-	ret = device_800_init(chip, pdata);
+	ret = device_800_init(chip);
 	if (ret) {
 		dev_err(chip->dev, "Failed to initialize 88pm800 devices\n");
 		goto err_device_init;
-- 
1.9.1

Re: [PATCH-v6 3/6] mfd: 88pm800: Get pdata from 'device' rather than passing as a parameter

[Lee Jones]

On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:

> Currently the device_xxx_init() fns take pdata as an input parameter to
> the fn, but the cleaner approach would be to use dev_get_platdata() to
> get the pdata.
> So this patch changes the code accordingly.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 95c418c..af8232f 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -254,11 +254,11 @@ static const struct regmap_irq pm800_irqs[] = {
>  	},
>  };
>  
> -static int device_gpadc_init(struct pm80x_chip *chip,
> -				       struct pm80x_platform_data *pdata)
> +static int device_gpadc_init(struct pm80x_chip *chip)
>  {
>  	struct pm80x_subchip *subchip = chip->subchip;
>  	struct regmap *map = subchip->regmap_gpadc;
> +	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
>  	int data = 0, mask = 0, ret = 0;
>  
>  	if (!map) {
> @@ -320,17 +320,17 @@ out:
>  	return ret;
>  }
>  
> -static int device_onkey_init(struct pm80x_chip *chip,
> -				struct pm80x_platform_data *pdata)
> +static int device_onkey_init(struct pm80x_chip *chip)
>  {
>  	return mfd_add_devices(chip->dev, 0, &onkey_devs[0],
>  			      ARRAY_SIZE(onkey_devs), &onkey_resources[0], 0,
>  			      NULL);
>  }
>  
> -static int device_rtc_init(struct pm80x_chip *chip,
> -				struct pm80x_platform_data *pdata)
> +static int device_rtc_init(struct pm80x_chip *chip)
>  {
> +	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
> +
>  	if (pdata) {
>  		rtc_devs[0].platform_data = pdata->rtc;
>  		rtc_devs[0].pdata_size =
> @@ -341,8 +341,7 @@ static int device_rtc_init(struct pm80x_chip *chip,
>  			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
>  }
>  
> -static int device_regulator_init(struct pm80x_chip *chip,
> -					   struct pm80x_platform_data *pdata)
> +static int device_regulator_init(struct pm80x_chip *chip)
>  {
>  	return mfd_add_devices(chip->dev, 0, &regulator_devs[0],
>  			      ARRAY_SIZE(regulator_devs), NULL, 0, NULL);
> @@ -463,11 +462,11 @@ static void pm800_pages_exit(struct pm80x_chip *chip)
>  		i2c_unregister_device(subchip->gpadc_page);
>  }
>  
> -static int device_800_init(struct pm80x_chip *chip,
> -				     struct pm80x_platform_data *pdata)
> +static int device_800_init(struct pm80x_chip *chip)
>  {
>  	int ret;
>  	unsigned int val;
> +	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
>  
>  	/*
>  	 * alarm wake up bit will be clear in device_irq_init(),
> @@ -483,7 +482,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  			pdata->rtc->rtc_wakeup = 1;
>  	}
>  
> -	ret = device_gpadc_init(chip, pdata);
> +	ret = device_gpadc_init(chip);
>  	if (ret < 0) {
>  		dev_err(chip->dev, "[%s]Failed to init gpadc\n", __func__);
>  		goto out;
> @@ -497,19 +496,19 @@ static int device_800_init(struct pm80x_chip *chip,
>  		goto out;
>  	}
>  
> -	ret = device_onkey_init(chip, pdata);
> +	ret = device_onkey_init(chip);
>  	if (ret) {
>  		dev_err(chip->dev, "Failed to add onkey subdev\n");
>  		goto out_dev;
>  	}
>  
> -	ret = device_rtc_init(chip, pdata);
> +	ret = device_rtc_init(chip);
>  	if (ret) {
>  		dev_err(chip->dev, "Failed to add rtc subdev\n");
>  		goto out;
>  	}
>  
> -	ret = device_regulator_init(chip, pdata);
> +	ret = device_regulator_init(chip);
>  	if (ret) {
>  		dev_err(chip->dev, "Failed to add regulators subdev\n");
>  		goto out;
> @@ -575,7 +574,7 @@ static int pm800_probe(struct i2c_client *client,
>  		goto err_device_init;
>  	}
>  
> -	ret = device_800_init(chip, pdata);
> +	ret = device_800_init(chip);
>  	if (ret) {
>  		dev_err(chip->dev, "Failed to initialize 88pm800 devices\n");
>  		goto err_device_init;

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

[PATCH-v6 4/6] mfd: 88pm800: Remove unnecessary protection around pdata

[Vaibhav Hiremath]

With addition of proper checks in place in pm800_probe function,
which makes sure that pdata would never become NULL.
So remove all unnecessary protection around pdata in whole
driver code.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/88pm800.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index af8232f..074ba8b 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -302,7 +302,7 @@ static int device_gpadc_init(struct pm80x_chip *chip)
 	mask = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
 		PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
 
-	if (pdata && (pdata->batt_det == 0))
+	if (pdata->batt_det == 0)
 		data = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
 			PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
 	else
@@ -331,11 +331,8 @@ static int device_rtc_init(struct pm80x_chip *chip)
 {
 	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
 
-	if (pdata) {
-		rtc_devs[0].platform_data = pdata->rtc;
-		rtc_devs[0].pdata_size =
-				pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
-	}
+	rtc_devs[0].platform_data = pdata->rtc;
+	rtc_devs[0].pdata_size = pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
 
 	return mfd_add_devices(chip->dev, 0, &rtc_devs[0],
 			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
@@ -478,7 +475,7 @@ static int device_800_init(struct pm80x_chip *chip)
 		goto out;
 	}
 	if (val & PM800_ALARM_WAKEUP) {
-		if (pdata && pdata->rtc)
+		if (pdata->rtc)
 			pdata->rtc->rtc_wakeup = 1;
 	}
 
@@ -580,7 +577,7 @@ static int pm800_probe(struct i2c_client *client,
 		goto err_device_init;
 	}
 
-	if (pdata && pdata->plat_config)
+	if (pdata->plat_config)
 		pdata->plat_config(chip, pdata);
 
 	return 0;
-- 
1.9.1

[PATCH-v6 5/6] 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 and removed unnecessary field
pm80x_chip.irq_mode, using platform_data.irq_clr_method.

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

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 074ba8b..95c8ad4 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -347,8 +347,9 @@ static int device_regulator_init(struct pm80x_chip *chip)
 static int device_irq_init_800(struct pm80x_chip *chip)
 {
 	struct regmap *map = chip->regmap;
+	struct pm80x_platform_data *pdata = dev_get_platdata(chip->dev);
 	unsigned long flags = IRQF_ONESHOT;
-	int data, mask, ret = -EINVAL;
+	int irq_clr_mode, mask, ret = -EINVAL;
 
 	if (!map || !chip->irq) {
 		dev_err(chip->dev, "incorrect parameters\n");
@@ -356,15 +357,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 	}
 
 	/*
-	 * irq_mode defines the way of clearing interrupt. it's read-clear by
-	 * default.
+	 * irq_clr_on_wr defines the way of clearing interrupt by
+	 * read/write(0/1).  It's read-clear by default.
 	 */
 	mask =
 	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
 	    PM800_WAKEUP2_INT_MASK;
 
-	data = PM800_WAKEUP2_INT_CLEAR;
-	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
+	irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
 
 	if (ret < 0)
 		goto out;
@@ -541,6 +543,9 @@ static int pm800_probe(struct i2c_client *client,
 
 		/* Ensure we only alloc platform data once */
 		client->dev.platform_data = pdata;
+
+		/* by default, set irq clear method on write */
+		pdata->irq_clr_method = PM800_IRQ_CLR_ON_WRITE;
 	}
 
 	ret = pm80x_init(client);
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 8fcad63..9c5773b 100644
--- a/include/linux/mfd/88pm80x.h
+++ b/include/linux/mfd/88pm80x.h
@@ -77,6 +77,8 @@ enum {
 #define PM800_WAKEUP2			(0x0E)
 #define PM800_WAKEUP2_INV_INT		BIT(0)
 #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
+#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
 #define PM800_WAKEUP2_INT_MASK		BIT(2)
 
 #define PM800_POWER_UP_LOG		(0x10)
@@ -300,11 +302,14 @@ struct pm80x_chip {
 	struct regmap_irq_chip_data *irq_data;
 	int type;
 	int irq;
-	int irq_mode;
 	unsigned long wu_flag;
 	spinlock_t lock;
 };
 
+/* Used by irq_clr_method */
+#define PM800_IRQ_CLR_ON_READ	0
+#define PM800_IRQ_CLR_ON_WRITE	1
+
 struct pm80x_platform_data {
 	struct pm80x_rtc_pdata *rtc;
 	/*
@@ -315,7 +320,7 @@ struct pm80x_platform_data {
 	 */
 	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
 	unsigned int num_regulators;
-	int irq_mode;		/* Clear interrupt by read/write(0/1) */
+	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
 	int batt_det;		/* enable/disable */
 	int (*plat_config)(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata);
-- 
1.9.1

Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:

> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> (page 0) controls the method of clearing interrupt
> status of 88pm800 family of devices;
> 
>   0: clear on read
>   1: clear on write
> 
> If pdata is not coming from board file, then set the
> default irq clear method to "irq clear on write"
> 
> Also, as suggested by "Lee Jones" renaming variable field
> to appropriate name and removed unnecessary field
> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
> 
> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
>  include/linux/mfd/88pm80x.h |  9 +++++++--
>  2 files changed, 17 insertions(+), 7 deletions(-)

[...]

> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)

Use BIT().

> +/* Used by irq_clr_method */
> +#define PM800_IRQ_CLR_ON_READ	0
> +#define PM800_IRQ_CLR_ON_WRITE	1

> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
> +	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */

> +	irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);

This is pretty convoluted.

For starters you're abusing the 'bool' type here.  Bool is either
'true' or 'false', so at the very least you should rename
'irq_clr_method' to 'irq_clr_on_write'.

Then you can do: 

	irq_clr_mode = pdata->irq_clr_on_write ?
		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;

However, what I suggest you really do is share
PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
the value through directly.

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

Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name and removed unnecessary field
>> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
>>
>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>   include/linux/mfd/88pm80x.h |  9 +++++++--
>>   2 files changed, 17 insertions(+), 7 deletions(-)
>
> [...]
>
>> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
>
> Use BIT().
>
>> +/* Used by irq_clr_method */
>> +#define PM800_IRQ_CLR_ON_READ	0
>> +#define PM800_IRQ_CLR_ON_WRITE	1
>
>> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
>> +	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
>
>> +	irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>
> This is pretty convoluted.
>
> For starters you're abusing the 'bool' type here.  Bool is either
> 'true' or 'false', so at the very least you should rename
> 'irq_clr_method' to 'irq_clr_on_write'.
>
> Then you can do:
>
> 	irq_clr_mode = pdata->irq_clr_on_write ?
> 		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>

We have discussed on this, and went back-n-forth.
I think if I remember correctly, one of the version was using
true/false then we decided to rename it to relevant macro.

If I am not wrong V4 version of this series is exactly same as what you
are referring to.


> However, what I suggest you really do is share
> PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
> the value through directly.
>

I think we discussed about this also, and the reason I recall here is,

we may need to control this from DT in the future so we decided to keep
it boolean in platform_data and have simple check before writing to
register.

And I think that was also another reason we introduced

/* Used by irq_clr_method */
#define PM800_IRQ_CLR_ON_READ   0
#define PM800_IRQ_CLR_ON_WRITE  1

(Earlier it was true/false in V4)

Thanks,
Vaibhav

Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:

> 
> 
> On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> >On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:
> >
> >>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>(page 0) controls the method of clearing interrupt
> >>status of 88pm800 family of devices;
> >>
> >>   0: clear on read
> >>   1: clear on write
> >>
> >>If pdata is not coming from board file, then set the
> >>default irq clear method to "irq clear on write"
> >>
> >>Also, as suggested by "Lee Jones" renaming variable field
> >>to appropriate name and removed unnecessary field
> >>pm80x_chip.irq_mode, using platform_data.irq_clr_method.
> >>
> >>Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> >>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>---
> >>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >>  include/linux/mfd/88pm80x.h |  9 +++++++--
> >>  2 files changed, 17 insertions(+), 7 deletions(-)
> >
> >[...]
> >
> >>+#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
> >>+#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
> >
> >Use BIT().
> >
> >>+/* Used by irq_clr_method */
> >>+#define PM800_IRQ_CLR_ON_READ	0
> >>+#define PM800_IRQ_CLR_ON_WRITE	1
> >
> >>-	int irq_mode;		/* Clear interrupt by read/write(0/1) */
> >>+	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
> >
> >>+	irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
> >>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >
> >This is pretty convoluted.
> >
> >For starters you're abusing the 'bool' type here.  Bool is either
> >'true' or 'false', so at the very least you should rename
> >'irq_clr_method' to 'irq_clr_on_write'.
> >
> >Then you can do:
> >
> >	irq_clr_mode = pdata->irq_clr_on_write ?
> >		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >
> 
> We have discussed on this, and went back-n-forth.
> I think if I remember correctly, one of the version was using
> true/false then we decided to rename it to relevant macro.
> 
> If I am not wrong V4 version of this series is exactly same as what you
> are referring to.

Right.  I made a few suggestions which vary in usefulness depending on
how you plan to implement all of this.  Unfortunately this is a bit of
a bastardised version where some of it make sense and other parts
could do with some improvement.

> >However, what I suggest you really do is share
> >PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
> >the value through directly.
> >
> 
> I think we discussed about this also, and the reason I recall here is,
> 
> we may need to control this from DT in the future so we decided to keep
> it boolean in platform_data and have simple check before writing to
> register.
> 
> And I think that was also another reason we introduced
> 
> /* Used by irq_clr_method */
> #define PM800_IRQ_CLR_ON_READ   0
> #define PM800_IRQ_CLR_ON_WRITE  1

I think these are still required.  So it would look like this:

== Platform data ==

struct pdata {
  bool clear_irq_on_write;
};

pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};

== Driver ==

irq_clr_mode = pdata->clear_irq_on_write ?
                 PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);

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

Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
> On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
>>> On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:
>>>
>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>> (page 0) controls the method of clearing interrupt
>>>> status of 88pm800 family of devices;
>>>>
>>>>    0: clear on read
>>>>    1: clear on write
>>>>
>>>> If pdata is not coming from board file, then set the
>>>> default irq clear method to "irq clear on write"
>>>>
>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>> to appropriate name and removed unnecessary field
>>>> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
>>>>
>>>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>> ---
>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>   include/linux/mfd/88pm80x.h |  9 +++++++--
>>>>   2 files changed, 17 insertions(+), 7 deletions(-)
>>>
>>> [...]
>>>
>>>> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
>>>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
>>>
>>> Use BIT().
>>>
>>>> +/* Used by irq_clr_method */
>>>> +#define PM800_IRQ_CLR_ON_READ	0
>>>> +#define PM800_IRQ_CLR_ON_WRITE	1
>>>
>>>> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
>>>> +	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
>>>
>>>> +	irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>
>>> This is pretty convoluted.
>>>
>>> For starters you're abusing the 'bool' type here.  Bool is either
>>> 'true' or 'false', so at the very least you should rename
>>> 'irq_clr_method' to 'irq_clr_on_write'.
>>>
>>> Then you can do:
>>>
>>> 	irq_clr_mode = pdata->irq_clr_on_write ?
>>> 		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>
>>
>> We have discussed on this, and went back-n-forth.
>> I think if I remember correctly, one of the version was using
>> true/false then we decided to rename it to relevant macro.
>>
>> If I am not wrong V4 version of this series is exactly same as what you
>> are referring to.
>
> Right.  I made a few suggestions which vary in usefulness depending on
> how you plan to implement all of this.  Unfortunately this is a bit of
> a bastardised version where some of it make sense and other parts
> could do with some improvement.
>

This so called "basterdised version could have been avoided :)

V2 version itself was clean and ready. It just got dragged into
multiple iterations.

>>> However, what I suggest you really do is share
>>> PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
>>> the value through directly.
>>>
>>
>> I think we discussed about this also, and the reason I recall here is,
>>
>> we may need to control this from DT in the future so we decided to keep
>> it boolean in platform_data and have simple check before writing to
>> register.
>>
>> And I think that was also another reason we introduced
>>
>> /* Used by irq_clr_method */
>> #define PM800_IRQ_CLR_ON_READ   0
>> #define PM800_IRQ_CLR_ON_WRITE  1
>
> I think these are still required.  So it would look like this:
>

NO. I think you are confused here,
We have two different macros playing around here,


+/* Used by irq_clr_method */
+#define PM800_IRQ_CLR_ON_READ	0
+#define PM800_IRQ_CLR_ON_WRITE	1

/* Used to write to register */
+#define PM800_WAKEUP2_INT_READ_CLEAR		(0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR		(1 << 1)



> == Platform data ==
>
> struct pdata {
>    bool clear_irq_on_write;
> };
>
> pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};
>
> == Driver ==
>
> irq_clr_mode = pdata->clear_irq_on_write ?
>                   PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>

Please check V2, which is exactly same as above.

https://patchwork.kernel.org/patch/6627781/


If you are OK with it, I will spin another version and submit it.

Thanks,
Vaibhav

Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:

> 
> 
> On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
> >On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
> >
> >>
> >>
> >>On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> >>>On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:
> >>>
> >>>>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>>>(page 0) controls the method of clearing interrupt
> >>>>status of 88pm800 family of devices;
> >>>>
> >>>>   0: clear on read
> >>>>   1: clear on write
> >>>>
> >>>>If pdata is not coming from board file, then set the
> >>>>default irq clear method to "irq clear on write"
> >>>>
> >>>>Also, as suggested by "Lee Jones" renaming variable field
> >>>>to appropriate name and removed unnecessary field
> >>>>pm80x_chip.irq_mode, using platform_data.irq_clr_method.
> >>>>
> >>>>Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> >>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>>>---
> >>>>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >>>>  include/linux/mfd/88pm80x.h |  9 +++++++--
> >>>>  2 files changed, 17 insertions(+), 7 deletions(-)
> >>>
> >>>[...]
> >>>
> >>>>+#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
> >>>>+#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
> >>>
> >>>Use BIT().
> >>>
> >>>>+/* Used by irq_clr_method */
> >>>>+#define PM800_IRQ_CLR_ON_READ	0
> >>>>+#define PM800_IRQ_CLR_ON_WRITE	1
> >>>
> >>>>-	int irq_mode;		/* Clear interrupt by read/write(0/1) */
> >>>>+	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
> >>>
> >>>>+	irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
> >>>>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>>>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >>>
> >>>This is pretty convoluted.
> >>>
> >>>For starters you're abusing the 'bool' type here.  Bool is either
> >>>'true' or 'false', so at the very least you should rename
> >>>'irq_clr_method' to 'irq_clr_on_write'.
> >>>
> >>>Then you can do:
> >>>
> >>>	irq_clr_mode = pdata->irq_clr_on_write ?
> >>>		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>>
> >>
> >>We have discussed on this, and went back-n-forth.
> >>I think if I remember correctly, one of the version was using
> >>true/false then we decided to rename it to relevant macro.
> >>
> >>If I am not wrong V4 version of this series is exactly same as what you
> >>are referring to.
> >
> >Right.  I made a few suggestions which vary in usefulness depending on
> >how you plan to implement all of this.  Unfortunately this is a bit of
> >a bastardised version where some of it make sense and other parts
> >could do with some improvement.
> >
> 
> This so called "basterdised version could have been avoided :)
> 
> V2 version itself was clean and ready. It just got dragged into
> multiple iterations.

Don't kid yourself.  There were still improvements to be made.

> >>>However, what I suggest you really do is share
> >>>PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
> >>>the value through directly.
> >>>
> >>
> >>I think we discussed about this also, and the reason I recall here is,
> >>
> >>we may need to control this from DT in the future so we decided to keep
> >>it boolean in platform_data and have simple check before writing to
> >>register.
> >>
> >>And I think that was also another reason we introduced
> >>
> >>/* Used by irq_clr_method */
> >>#define PM800_IRQ_CLR_ON_READ   0
> >>#define PM800_IRQ_CLR_ON_WRITE  1
> >
> >I think these are still required.  So it would look like this:
> >
> 
> NO. I think you are confused here,
> We have two different macros playing around here,
> 
> 
> +/* Used by irq_clr_method */
> +#define PM800_IRQ_CLR_ON_READ	0
> +#define PM800_IRQ_CLR_ON_WRITE	1
> 
> /* Used to write to register */
> +#define PM800_WAKEUP2_INT_READ_CLEAR		(0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR		(1 << 1)

I know.  I used both of them *correctly* in my example below.  No
confusion here.

> >== Platform data ==
> >
> >struct pdata {
> >   bool clear_irq_on_write;
> >};
> >
> >pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};
> >
> >== Driver ==
> >
> >irq_clr_mode = pdata->clear_irq_on_write ?
> >                  PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >
> 
> Please check V2, which is exactly same as above.
> 
> https://patchwork.kernel.org/patch/6627781/
> 
> 
> If you are OK with it, I will spin another version and submit it.

If you can't use the value directly, which if you want to pull the
value from DT you can't, then either use the method above, or
something like this might be better:

int clear_on_write = 0;

if (pdata->clear_irq_on_write)
   clear_on_write = PM800_WAKEUP2_INT_WRITE_CLEAR;

.. this way you only need to add one new define and you can drop
PM800_WAKEUP2_INT_READ_CLEAR altogether.  This is better, because it
will aid you to move to the BIT() macro easier (there is no BIT()
value for shifting 0's).

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

Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

[Vaibhav Hiremath]

On Tuesday 25 August 2015 02:00 PM, Lee Jones wrote:
> On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
>>> On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
>>>
>>>>
>>>>
>>>> On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
>>>>> On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>>> (page 0) controls the method of clearing interrupt
>>>>>> status of 88pm800 family of devices;
>>>>>>
>>>>>>    0: clear on read
>>>>>>    1: clear on write
>>>>>>
>>>>>> If pdata is not coming from board file, then set the
>>>>>> default irq clear method to "irq clear on write"
>>>>>>
>>>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>>>> to appropriate name and removed unnecessary field
>>>>>> pm80x_chip.irq_mode, using platform_data.irq_clr_method.
>>>>>>
>>>>>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>>>> ---
>>>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>>>   include/linux/mfd/88pm80x.h |  9 +++++++--
>>>>>>   2 files changed, 17 insertions(+), 7 deletions(-)
>>>>>
>>>>> [...]
>>>>>
>>>>>> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
>>>>>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
>>>>>
>>>>> Use BIT().
>>>>>
>>>>>> +/* Used by irq_clr_method */
>>>>>> +#define PM800_IRQ_CLR_ON_READ	0
>>>>>> +#define PM800_IRQ_CLR_ON_WRITE	1
>>>>>
>>>>>> -	int irq_mode;		/* Clear interrupt by read/write(0/1) */
>>>>>> +	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
>>>>>
>>>>>> +	irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
>>>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>>>
>>>>> This is pretty convoluted.
>>>>>
>>>>> For starters you're abusing the 'bool' type here.  Bool is either
>>>>> 'true' or 'false', so at the very least you should rename
>>>>> 'irq_clr_method' to 'irq_clr_on_write'.
>>>>>
>>>>> Then you can do:
>>>>>
>>>>> 	irq_clr_mode = pdata->irq_clr_on_write ?
>>>>> 		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>>>
>>>>
>>>> We have discussed on this, and went back-n-forth.
>>>> I think if I remember correctly, one of the version was using
>>>> true/false then we decided to rename it to relevant macro.
>>>>
>>>> If I am not wrong V4 version of this series is exactly same as what you
>>>> are referring to.
>>>
>>> Right.  I made a few suggestions which vary in usefulness depending on
>>> how you plan to implement all of this.  Unfortunately this is a bit of
>>> a bastardised version where some of it make sense and other parts
>>> could do with some improvement.
>>>
>>
>> This so called "basterdised version could have been avoided :)
>>
>> V2 version itself was clean and ready. It just got dragged into
>> multiple iterations.
>
> Don't kid yourself.  There were still improvements to be made.
>

Yes indeed,
Moving to pdata was required. I was referring to logic part of it.

>>>>> However, what I suggest you really do is share
>>>>> PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass
>>>>> the value through directly.
>>>>>
>>>>
>>>> I think we discussed about this also, and the reason I recall here is,
>>>>
>>>> we may need to control this from DT in the future so we decided to keep
>>>> it boolean in platform_data and have simple check before writing to
>>>> register.
>>>>
>>>> And I think that was also another reason we introduced
>>>>
>>>> /* Used by irq_clr_method */
>>>> #define PM800_IRQ_CLR_ON_READ   0
>>>> #define PM800_IRQ_CLR_ON_WRITE  1
>>>
>>> I think these are still required.  So it would look like this:
>>>
>>
>> NO. I think you are confused here,
>> We have two different macros playing around here,
>>
>>
>> +/* Used by irq_clr_method */
>> +#define PM800_IRQ_CLR_ON_READ	0
>> +#define PM800_IRQ_CLR_ON_WRITE	1
>>
>> /* Used to write to register */
>> +#define PM800_WAKEUP2_INT_READ_CLEAR		(0 << 1)
>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR		(1 << 1)
>
> I know.  I used both of them *correctly* in my example below.  No
> confusion here.
>
>>> == Platform data ==
>>>
>>> struct pdata {
>>>    bool clear_irq_on_write;
>>> };
>>>
>>> pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE};
>>>
>>> == Driver ==
>>>
>>> irq_clr_mode = pdata->clear_irq_on_write ?
>>>                   PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>> regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>
>>


The V2 version had


	irq_clr_mode = (chip->irq_clr_on_wr) ?
		PM800_WAKEUP2_INT_WRITE_CLEAR :
		PM800_WAKEUP2_INT_READ_CLEAR;
	ret = regmap_update_bits(map, PM800_WAKEUP2, mask,
						irq_clr_mode);

Which is exactly same as your example above, except pdata moment.

Lets not discuss too much on this, I think its time for conclusion :)


Your below example looks even better to me. So I will adopt below
example and resubmit the series.

>> Please check V2, which is exactly same as above.
>>
>> https://patchwork.kernel.org/patch/6627781/
>>
>>
>> If you are OK with it, I will spin another version and submit it.
>
> If you can't use the value directly, which if you want to pull the
> value from DT you can't, then either use the method above, or
> something like this might be better:
>
> int clear_on_write = 0;
>
> if (pdata->clear_irq_on_write)
>     clear_on_write = PM800_WAKEUP2_INT_WRITE_CLEAR;
>
> .. this way you only need to add one new define and you can drop
> PM800_WAKEUP2_INT_READ_CLEAR altogether.  This is better, because it
> will aid you to move to the BIT() macro easier (there is no BIT()
> value for shifting 0's).
>

Just to clarify, I will adopt this implementation.

Thanks,
Vaibhav

Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method

[Lee Jones]

On Tue, 25 Aug 2015, Vaibhav Hiremath wrote:
> On Tuesday 25 August 2015 02:00 PM, Lee Jones wrote:
> >On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
> >>On Monday 24 August 2015 09:21 PM, Lee Jones wrote:
> >>>On Mon, 24 Aug 2015, Vaibhav Hiremath wrote:
> >>>>On Monday 24 August 2015 07:24 PM, Lee Jones wrote:
> >>>>>On Wed, 08 Jul 2015, Vaibhav Hiremath wrote:
> >>>>>
> >>>>>>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>>>>>(page 0) controls the method of clearing interrupt
> >>>>>>status of 88pm800 family of devices;
> >>>>>>
> >>>>>>   0: clear on read
> >>>>>>   1: clear on write
> >>>>>>
> >>>>>>If pdata is not coming from board file, then set the
> >>>>>>default irq clear method to "irq clear on write"
> >>>>>>
> >>>>>>Also, as suggested by "Lee Jones" renaming variable field
> >>>>>>to appropriate name and removed unnecessary field
> >>>>>>pm80x_chip.irq_mode, using platform_data.irq_clr_method.
> >>>>>>
> >>>>>>Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> >>>>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>>>Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>>>>>---
> >>>>>>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >>>>>>  include/linux/mfd/88pm80x.h |  9 +++++++--
> >>>>>>  2 files changed, 17 insertions(+), 7 deletions(-)

[...]

> >>If you are OK with it, I will spin another version and submit it.
> >
> >If you can't use the value directly, which if you want to pull the
> >value from DT you can't, then either use the method above, or
> >something like this might be better:
> >
> >int clear_on_write = 0;
> >
> >if (pdata->clear_irq_on_write)
> >    clear_on_write = PM800_WAKEUP2_INT_WRITE_CLEAR;
> >
> >.. this way you only need to add one new define and you can drop
> >PM800_WAKEUP2_INT_READ_CLEAR altogether.  This is better, because it
> >will aid you to move to the BIT() macro easier (there is no BIT()
> >value for shifting 0's).
> >
> 
> Just to clarify, I will adopt this implementation.

Sounds good.  Although, I would suggest just using 'val' as the local
variable.  It makes it more clear that PM800_WAKEUP2_INT_WRITE_CLEAR
is a bit value.

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

[PATCH-v6 6/6] 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>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt

diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
new file mode 100644
index 0000000..dec842f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -0,0 +1,53 @@
+* Marvell 88PM80x Power Management IC
+
+Required parent device properties:
+- compatible		: "marvell,88pm800", "marvell,88pm805", "marvell,88pm860"
+- reg			: the I2C slave address for the 88pm80x family chip
+- interrupts		: IRQ line for the 88pm80x family chip
+- interrupt-controller	: describes the 88pm80x family chip as an interrupt
+			  controller
+- #interrupt-cells 	: should be 1.
+			  The cell is the 88pm80x local IRQ number
+
+88pm80x family of devices consists of varied group of sub-devices:
+
+Device		 	Supply Names	 Description
+------		 	------------	 -----------
+88pm80x-onkey		:		: On key
+88pm80x-rtc		:		: RTC
+88pm80x-regulator	:		: Regulators
+
+Example:
+
+	pmic: 88pm800@30 {
+		compatible = "marvell,88pm800";
+		reg = <0x30>;
+		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-parent = <&gic>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		regulators {
+			compatible = "marvell,88pm80x-regulator";
+
+			buck1a: BUCK1A {
+				regulator-name = "BUCK1A";
+				regulator-min-microvolt = <600000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo1: LDO1 {
+				regulator-name = "LDO1";
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+
+		rtc {
+			compatible = "marvell,88pm80x-rtc";
+		};
+	};
-- 
1.9.1

Re: [PATCH-v6 0/6] mfd: 88pm800: Add Device tree support

[Vaibhav Hiremath]

On Wednesday 08 July 2015 05:56 PM, Vaibhav Hiremath wrote:
> 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
>
> V5 => V6
> =======

I hope this will be queued for 4.2

Thanks,
Vaibhav

> Link to V5: https://lkml.org/lkml/2015/6/29/283
>
>   - Added new patch to the series PATCH [1/6]
>     Cleanup patch to remove duplicate dev_err messages
>   - Added new patch to the series PACTH [3/6]
>     Cleanup patch to get pdata from 'device' pointer instead of
>     passing as a parameter.
>   - Removed irq_clr_mode/irq_mode field from 'struct pm80x_chip'
>     and use pdata.irq_clr_method to set irq clear method.
>   - Added acked-by and reviewed-by to respective patches.
>
> V4 => V5
> =======
> Link to V4: https://lkml.org/lkml/2015/6/25/67
>
>    - Renamed binding back again to 88pm800, as 'Yi Zhang' already started
>      submitting 88pm88x, so 88pm8xx won't make sense. Better name would be to
>      stick with 88pm80x.
>    - Added new patch to series PATCH 2/4, to remove unwanted protection around
>      padata
>    - As suggested by Lee, added macro based implementation for CLEAR_ON_WRITE
>      and CLEAR_ON_READ.
>    - and fixed other trivial comments.
>
> V3 => V4
> =======
> Link to V3: https://lkml.org/lkml/2015/6/24/143
>
>    -  irq clear method is 88PM800 feature, which is not dependent on board or
>       doesn't require any wiring changes, so DT is not the way.
>       Hardcoded to "irq clear on write" if board file doesn't exist.
>    - Updated binding patch (PATCH 3/3) to remove irq-clr-on-wr entry.
>    - Since PATCH 3/3 changed from original, removed Rob's Acked-by.
>
> V2 => V3
> =======
> Link to V2: https://www.mail-archive.com/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 (6):
>    mfd: 88pm800: remove duplicate dev_err calls during probe
>    mfd: 88pm800: Add device tree support
>    mfd: 88pm800: Get pdata from 'device' rather than passing as a
>      parameter
>    mfd: 88pm800: Remove unnecessary protection around pdata
>    mfd: 88pm800: Set default interrupt clear method
>    mfd: devicetree: bindings: Add new 88pm800 mfd binding
>
>   Documentation/devicetree/bindings/mfd/88pm800.txt |  53 ++++++++++
>   drivers/mfd/88pm800.c                             | 113 +++++++++++-----------
>   include/linux/mfd/88pm80x.h                       |   9 +-
>   3 files changed, 118 insertions(+), 57 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
>

Re: [PATCH-v6 0/6] mfd: 88pm800: Add Device tree support

[Vaibhav Hiremath]

On Tuesday 14 July 2015 12:27 AM, Vaibhav Hiremath wrote:
>
>
> On Wednesday 08 July 2015 05:56 PM, Vaibhav Hiremath wrote:
>> 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
>>
>> V5 => V6
>> =======
>
> I hope this will be queued for 4.2
>


Lee,

This patch-series is pending since long time, reviewed and acked.
Request to queue it up for next merge window.

Thanks,
Vaibhav