[PATCH-V5 0/4] mfd: 88pm800: Add Device tree support

[PATCH-V5 0/4] 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

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 (4):
  mfd: 88pm800: Add device tree support
  mfd: 88pm800: Remove unnecessary protection around pdata
  mfd: 88pm800: Set default interrupt clear method
  mfd: devicetree: bindings: Add new 88pm800 mfd binding

 Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
 drivers/mfd/88pm800.c                             | 52 ++++++++++++++++------
 include/linux/mfd/88pm80x.h                       | 10 ++++-
 3 files changed, 100 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt

-- 
1.9.1

[PATCH-V5 1/4] 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

Re: [PATCH-V5 1/4] mfd: 88pm800: Add device tree support

[Krzysztof Kozlowski]

2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> 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(+)

Reviewed-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

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

Re: [PATCH-V5 1/4] mfd: 88pm800: Add device tree support

[Lee Jones]

On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:

> Add DT support to the 88pm800 driver, along with compatible
> field for it's sub-devices (rtc, onkey and regulator)
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)

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

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

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

[PATCH-V5 2/4] 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mfd/88pm800.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 40fd014..d495737 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -302,7 +302,7 @@ static int device_gpadc_init(struct pm80x_chip *chip,
 	mask = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
 		PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
 
-	if (pdata && (pdata->batt_det == 0))
+	if (pdata->batt_det == 0)
 		data = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
 			PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
 	else
@@ -342,11 +342,9 @@ static int device_rtc_init(struct pm80x_chip *chip,
 {
 	int ret;
 
-	if (pdata) {
-		rtc_devs[0].platform_data = pdata->rtc;
-		rtc_devs[0].pdata_size =
-				pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
-	}
+	rtc_devs[0].platform_data = pdata->rtc;
+	rtc_devs[0].pdata_size = pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
+
 	ret = mfd_add_devices(chip->dev, 0, &rtc_devs[0],
 			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
 	if (ret) {
@@ -503,7 +501,7 @@ static int device_800_init(struct pm80x_chip *chip,
 		goto out;
 	}
 	if (val & PM800_ALARM_WAKEUP) {
-		if (pdata && pdata->rtc)
+		if (pdata->rtc)
 			pdata->rtc->rtc_wakeup = 1;
 	}
 
@@ -602,7 +600,7 @@ static int pm800_probe(struct i2c_client *client,
 		goto err_device_init;
 	}
 
-	if (pdata && pdata->plat_config)
+	if (pdata->plat_config)
 		pdata->plat_config(chip, pdata);
 
 	return 0;
-- 
1.9.1

--
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

Re: [PATCH-V5 2/4] mfd: 88pm800: Remove unnecessary protection around pdata

[Krzysztof Kozlowski]

2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/mfd/88pm800.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>

Reviewed-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

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

Re: [PATCH-V5 2/4] mfd: 88pm800: Remove unnecessary protection around pdata

[Lee Jones]

On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:

> With addition of proper checks in place in pm800_probe function,
> which makes sure that pdata would never become NULL.
> So remove all unnecessary protection around pdata in whole
> driver code.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/mfd/88pm800.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 40fd014..d495737 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -302,7 +302,7 @@ static int device_gpadc_init(struct pm80x_chip *chip,
>  	mask = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
>  		PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
>  
> -	if (pdata && (pdata->batt_det == 0))
> +	if (pdata->batt_det == 0)
>  		data = (PM800_GPADC_GP_BIAS_EN0 | PM800_GPADC_GP_BIAS_EN1 |
>  			PM800_GPADC_GP_BIAS_EN2 | PM800_GPADC_GP_BIAS_EN3);
>  	else
> @@ -342,11 +342,9 @@ static int device_rtc_init(struct pm80x_chip *chip,
>  {
>  	int ret;
>  
> -	if (pdata) {
> -		rtc_devs[0].platform_data = pdata->rtc;
> -		rtc_devs[0].pdata_size =
> -				pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
> -	}
> +	rtc_devs[0].platform_data = pdata->rtc;
> +	rtc_devs[0].pdata_size = pdata->rtc ? sizeof(struct pm80x_rtc_pdata) : 0;
> +
>  	ret = mfd_add_devices(chip->dev, 0, &rtc_devs[0],
>  			      ARRAY_SIZE(rtc_devs), NULL, 0, NULL);
>  	if (ret) {
> @@ -503,7 +501,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  		goto out;
>  	}
>  	if (val & PM800_ALARM_WAKEUP) {
> -		if (pdata && pdata->rtc)
> +		if (pdata->rtc)
>  			pdata->rtc->rtc_wakeup = 1;
>  	}
>  
> @@ -602,7 +600,7 @@ static int pm800_probe(struct i2c_client *client,
>  		goto err_device_init;
>  	}
>  
> -	if (pdata && pdata->plat_config)
> +	if (pdata->plat_config)
>  		pdata->plat_config(chip, pdata);
>  
>  	return 0;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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-V5 3/4] 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-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 | 10 ++++++++--
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index d495737..66347be 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -374,7 +374,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 {
 	struct regmap *map = chip->regmap;
 	unsigned long flags = IRQF_ONESHOT;
-	int data, mask, ret = -EINVAL;
+	int irq_clr_mode, mask, ret = -EINVAL;
 
 	if (!map || !chip->irq) {
 		dev_err(chip->dev, "incorrect parameters\n");
@@ -382,15 +382,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
 	}
 
 	/*
-	 * irq_mode defines the way of clearing interrupt. it's read-clear by
-	 * default.
+	 * irq_clr_on_wr defines the way of clearing interrupt by
+	 * read/write(0/1).  It's read-clear by default.
 	 */
 	mask =
 	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
 	    PM800_WAKEUP2_INT_MASK;
 
-	data = PM800_WAKEUP2_INT_CLEAR;
-	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
+	irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
 
 	if (ret < 0)
 		goto out;
@@ -512,6 +513,7 @@ static int device_800_init(struct pm80x_chip *chip,
 	}
 
 	chip->regmap_irq_chip = &pm800_irq_chip;
+	chip->irq_clr_method = pdata->irq_clr_method;
 
 	ret = device_irq_init_800(chip);
 	if (ret < 0) {
@@ -564,6 +566,9 @@ static int pm800_probe(struct i2c_client *client,
 		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
 		if (!pdata)
 			return -ENOMEM;
+
+		/* by default, set irq clear method on write */
+		pdata->irq_clr_method = PM800_IRQ_CLR_ON_WRITE;
 	}
 
 	ret = pm80x_init(client);
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 8fcad63..648e01a 100644
--- a/include/linux/mfd/88pm80x.h
+++ b/include/linux/mfd/88pm80x.h
@@ -77,6 +77,8 @@ enum {
 #define PM800_WAKEUP2			(0x0E)
 #define PM800_WAKEUP2_INV_INT		BIT(0)
 #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
+#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
+#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
 #define PM800_WAKEUP2_INT_MASK		BIT(2)
 
 #define PM800_POWER_UP_LOG		(0x10)
@@ -300,7 +302,11 @@ struct pm80x_chip {
 	struct regmap_irq_chip_data *irq_data;
 	int type;
 	int irq;
-	int irq_mode;
+
+#define PM800_IRQ_CLR_ON_READ	0
+#define PM800_IRQ_CLR_ON_WRITE	1
+
+	bool irq_clr_method;	/* '1': Clear on write, '0': Clear on read */
 	unsigned long wu_flag;
 	spinlock_t lock;
 };
@@ -315,7 +321,7 @@ struct pm80x_platform_data {
 	 */
 	struct regulator_init_data *regulators[PM800_ID_RG_MAX];
 	unsigned int num_regulators;
-	int irq_mode;		/* Clear interrupt by read/write(0/1) */
+	bool irq_clr_method;		/* Clear interrupt by read/write(0/1) */
 	int batt_det;		/* enable/disable */
 	int (*plat_config)(struct pm80x_chip *chip,
 				struct pm80x_platform_data *pdata);
-- 
1.9.1

--
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

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

[Krzysztof Kozlowski]

2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> 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 | 10 ++++++++--
>  2 files changed, 18 insertions(+), 7 deletions(-)
>

Looks fine to me.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

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

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

[Lee Jones]

On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:

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

What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
directly without all of this faffing about.

  regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);

>  	if (ret < 0)
>  		goto out;
> @@ -512,6 +513,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  	}
>  
>  	chip->regmap_irq_chip = &pm800_irq_chip;
> +	chip->irq_clr_method = pdata->irq_clr_method;
>  
>  	ret = device_irq_init_800(chip);
>  	if (ret < 0) {
> @@ -564,6 +566,9 @@ static int pm800_probe(struct i2c_client *client,
>  		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>  		if (!pdata)
>  			return -ENOMEM;
> +
> +		/* by default, set irq clear method on write */
> +		pdata->irq_clr_method = PM800_IRQ_CLR_ON_WRITE;
>  	}
>  
>  	ret = pm80x_init(client);
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 8fcad63..648e01a 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -77,6 +77,8 @@ enum {
>  #define PM800_WAKEUP2			(0x0E)
>  #define PM800_WAKEUP2_INV_INT		BIT(0)
>  #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
>  #define PM800_WAKEUP2_INT_MASK		BIT(2)
>  
>  #define PM800_POWER_UP_LOG		(0x10)
> @@ -300,7 +302,11 @@ struct pm80x_chip {
>  	struct regmap_irq_chip_data *irq_data;
>  	int type;
>  	int irq;
> -	int irq_mode;
> +
> +#define PM800_IRQ_CLR_ON_READ	0
> +#define PM800_IRQ_CLR_ON_WRITE	1

Defines in the middle of structs makes for ugly code.

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

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

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

[Vaibhav Hiremath]

On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
>
>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>> (page 0) controls the method of clearing interrupt
>> status of 88pm800 family of devices;
>>
>>    0: clear on read
>>    1: clear on write
>>
>> If pdata is not coming from board file, then set the
>> default irq clear method to "irq clear on write"
>>
>> Also, as suggested by "Lee Jones" renaming variable field
>> to appropriate name.
>>
>> Signed-off-by: Zhao Ye <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 | 10 ++++++++--
>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index d495737..66347be 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -374,7 +374,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   {
>>   	struct regmap *map = chip->regmap;
>>   	unsigned long flags = IRQF_ONESHOT;
>> -	int data, mask, ret = -EINVAL;
>> +	int irq_clr_mode, mask, ret = -EINVAL;
>>
>>   	if (!map || !chip->irq) {
>>   		dev_err(chip->dev, "incorrect parameters\n");
>> @@ -382,15 +382,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>   	}
>>
>>   	/*
>> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
>> -	 * default.
>> +	 * irq_clr_on_wr defines the way of clearing interrupt by
>> +	 * read/write(0/1).  It's read-clear by default.
>>   	 */
>>   	mask =
>>   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>   	    PM800_WAKEUP2_INT_MASK;
>>
>> -	data = PM800_WAKEUP2_INT_CLEAR;
>> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>> +	irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>
> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> directly without all of this faffing about.
>
>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>

Because "irq_clr_method" is of boolean type.
And macros which you are referring to is,

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


And also, I feel it is more cleaner approach with the current code as
register definition and userflag are maintained separately.

>>   	if (ret < 0)
>>   		goto out;
>> @@ -512,6 +513,7 @@ static int device_800_init(struct pm80x_chip *chip,
>>   	}
>>
>>   	chip->regmap_irq_chip = &pm800_irq_chip;
>> +	chip->irq_clr_method = pdata->irq_clr_method;
>>
>>   	ret = device_irq_init_800(chip);
>>   	if (ret < 0) {
>> @@ -564,6 +566,9 @@ static int pm800_probe(struct i2c_client *client,
>>   		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>   		if (!pdata)
>>   			return -ENOMEM;
>> +
>> +		/* by default, set irq clear method on write */
>> +		pdata->irq_clr_method = PM800_IRQ_CLR_ON_WRITE;
>>   	}
>>
>>   	ret = pm80x_init(client);
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index 8fcad63..648e01a 100644
>> --- a/include/linux/mfd/88pm80x.h
>> +++ b/include/linux/mfd/88pm80x.h
>> @@ -77,6 +77,8 @@ enum {
>>   #define PM800_WAKEUP2			(0x0E)
>>   #define PM800_WAKEUP2_INV_INT		BIT(0)
>>   #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
>> +#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
>>   #define PM800_WAKEUP2_INT_MASK		BIT(2)
>>
>>   #define PM800_POWER_UP_LOG		(0x10)
>> @@ -300,7 +302,11 @@ struct pm80x_chip {
>>   	struct regmap_irq_chip_data *irq_data;
>>   	int type;
>>   	int irq;
>> -	int irq_mode;
>> +
>> +#define PM800_IRQ_CLR_ON_READ	0
>> +#define PM800_IRQ_CLR_ON_WRITE	1
>
> Defines in the middle of structs makes for ugly code.
>

Sorry, but kernel code is full of such implementations.
Infact it is right place in terms of readability.

If you still feel insist to fix it, please let me know whether you want
to submit V6 just for this. Or you will fix it while merging.
I am OK with anything here...


Thanks,
Vaibhav
--
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

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

[Lee Jones]

On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
> >
> >>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>(page 0) controls the method of clearing interrupt
> >>status of 88pm800 family of devices;
> >>
> >>   0: clear on read
> >>   1: clear on write
> >>
> >>If pdata is not coming from board file, then set the
> >>default irq clear method to "irq clear on write"
> >>
> >>Also, as suggested by "Lee Jones" renaming variable field
> >>to appropriate name.
> >>
> >>Signed-off-by: Zhao Ye <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 | 10 ++++++++--
> >>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>index d495737..66347be 100644
> >>--- a/drivers/mfd/88pm800.c
> >>+++ b/drivers/mfd/88pm800.c
> >>@@ -374,7 +374,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> >>  {
> >>  	struct regmap *map = chip->regmap;
> >>  	unsigned long flags = IRQF_ONESHOT;
> >>-	int data, mask, ret = -EINVAL;
> >>+	int irq_clr_mode, mask, ret = -EINVAL;
> >>
> >>  	if (!map || !chip->irq) {
> >>  		dev_err(chip->dev, "incorrect parameters\n");
> >>@@ -382,15 +382,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> >>  	}
> >>
> >>  	/*
> >>-	 * irq_mode defines the way of clearing interrupt. it's read-clear by
> >>-	 * default.
> >>+	 * irq_clr_on_wr defines the way of clearing interrupt by
> >>+	 * read/write(0/1).  It's read-clear by default.
> >>  	 */
> >>  	mask =
> >>  	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
> >>  	    PM800_WAKEUP2_INT_MASK;
> >>
> >>-	data = PM800_WAKEUP2_INT_CLEAR;
> >>-	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> >>+	irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
> >>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >
> >What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> >PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> >directly without all of this faffing about.
> >
> >   regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
> >
> 
> Because "irq_clr_method" is of boolean type.
> And macros which you are referring to is,
> 
> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
> 
> 
> And also, I feel it is more cleaner approach with the current code as
> register definition and userflag are maintained separately.

I see your point, although it's a shame we have to have this code in
its place.

One thing I think you can do though is rid chip->irq_clr_method, just
use the one you already have in pdata.

> >>  	if (ret < 0)
> >>  		goto out;
> >>@@ -512,6 +513,7 @@ static int device_800_init(struct pm80x_chip *chip,
> >>  	}
> >>
> >>  	chip->regmap_irq_chip = &pm800_irq_chip;
> >>+	chip->irq_clr_method = pdata->irq_clr_method;
> >>
> >>  	ret = device_irq_init_800(chip);
> >>  	if (ret < 0) {
> >>@@ -564,6 +566,9 @@ static int pm800_probe(struct i2c_client *client,
> >>  		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>  		if (!pdata)
> >>  			return -ENOMEM;
> >>+
> >>+		/* by default, set irq clear method on write */
> >>+		pdata->irq_clr_method = PM800_IRQ_CLR_ON_WRITE;
> >>  	}
> >>
> >>  	ret = pm80x_init(client);
> >>diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> >>index 8fcad63..648e01a 100644
> >>--- a/include/linux/mfd/88pm80x.h
> >>+++ b/include/linux/mfd/88pm80x.h
> >>@@ -77,6 +77,8 @@ enum {
> >>  #define PM800_WAKEUP2			(0x0E)
> >>  #define PM800_WAKEUP2_INV_INT		BIT(0)
> >>  #define PM800_WAKEUP2_INT_CLEAR		BIT(1)
> >>+#define PM800_WAKEUP2_INT_READ_CLEAR	(0 << 1)
> >>+#define PM800_WAKEUP2_INT_WRITE_CLEAR	(1 << 1)
> >>  #define PM800_WAKEUP2_INT_MASK		BIT(2)
> >>
> >>  #define PM800_POWER_UP_LOG		(0x10)
> >>@@ -300,7 +302,11 @@ struct pm80x_chip {
> >>  	struct regmap_irq_chip_data *irq_data;
> >>  	int type;
> >>  	int irq;
> >>-	int irq_mode;
> >>+
> >>+#define PM800_IRQ_CLR_ON_READ	0
> >>+#define PM800_IRQ_CLR_ON_WRITE	1
> >
> >Defines in the middle of structs makes for ugly code.
> >
> 
> Sorry, but kernel code is full of such implementations.
> Infact it is right place in terms of readability.
> 
> If you still feel insist to fix it, please let me know whether you want
> to submit V6 just for this. Or you will fix it while merging.
> I am OK with anything here...
> 
> 
> Thanks,
> Vaibhav

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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

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

[Vaibhav Hiremath]

On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>> On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
>>>
>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>> (page 0) controls the method of clearing interrupt
>>>> status of 88pm800 family of devices;
>>>>
>>>>    0: clear on read
>>>>    1: clear on write
>>>>
>>>> If pdata is not coming from board file, then set the
>>>> default irq clear method to "irq clear on write"
>>>>
>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>> to appropriate name.
>>>>
>>>> Signed-off-by: Zhao Ye <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 | 10 ++++++++--
>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>> index d495737..66347be 100644
>>>> --- a/drivers/mfd/88pm800.c
>>>> +++ b/drivers/mfd/88pm800.c
>>>> @@ -374,7 +374,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>>>   {
>>>>   	struct regmap *map = chip->regmap;
>>>>   	unsigned long flags = IRQF_ONESHOT;
>>>> -	int data, mask, ret = -EINVAL;
>>>> +	int irq_clr_mode, mask, ret = -EINVAL;
>>>>
>>>>   	if (!map || !chip->irq) {
>>>>   		dev_err(chip->dev, "incorrect parameters\n");
>>>> @@ -382,15 +382,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>>>   	}
>>>>
>>>>   	/*
>>>> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
>>>> -	 * default.
>>>> +	 * irq_clr_on_wr defines the way of clearing interrupt by
>>>> +	 * read/write(0/1).  It's read-clear by default.
>>>>   	 */
>>>>   	mask =
>>>>   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>>>   	    PM800_WAKEUP2_INT_MASK;
>>>>
>>>> -	data = PM800_WAKEUP2_INT_CLEAR;
>>>> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>>>> +	irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>
>>> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
>>> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
>>> directly without all of this faffing about.
>>>
>>>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>>>
>>
>> Because "irq_clr_method" is of boolean type.
>> And macros which you are referring to is,
>>
>> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
>> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
>>
>>
>> And also, I feel it is more cleaner approach with the current code as
>> register definition and userflag are maintained separately.
>
> I see your point, although it's a shame we have to have this code in
> its place.
>
> One thing I think you can do though is rid chip->irq_clr_method, just
> use the one you already have in pdata.
>

Looking at the current code,
Yes, this can be done, but I have to do some more changes around it,
to make code cleaner,

change the signature of

static int device_irq_init_800(struct pm80x_chip *chip)

TO

static int device_irq_init_800(struct pm80x_chip *chip, struct 
pm80x_platform_data *pdata)


and then only use pdata->irq_clr_method.


How do you want to get this inside? V6 version? or separate patch?

I have one more cleanup patch in the queue, which I am planning to
submit today, if you are ok then I can submit along with that.


Thanks,
Vaibhav
--
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

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

[Lee Jones]

On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> >On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >>>On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
> >>>
> >>>>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>>>(page 0) controls the method of clearing interrupt
> >>>>status of 88pm800 family of devices;
> >>>>
> >>>>   0: clear on read
> >>>>   1: clear on write
> >>>>
> >>>>If pdata is not coming from board file, then set the
> >>>>default irq clear method to "irq clear on write"
> >>>>
> >>>>Also, as suggested by "Lee Jones" renaming variable field
> >>>>to appropriate name.
> >>>>
> >>>>Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> >>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>---
> >>>>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >>>>  include/linux/mfd/88pm80x.h | 10 ++++++++--
> >>>>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>>>
> >>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>index d495737..66347be 100644
> >>>>--- a/drivers/mfd/88pm800.c
> >>>>+++ b/drivers/mfd/88pm800.c
> >>>>@@ -374,7 +374,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> >>>>  {
> >>>>  	struct regmap *map = chip->regmap;
> >>>>  	unsigned long flags = IRQF_ONESHOT;
> >>>>-	int data, mask, ret = -EINVAL;
> >>>>+	int irq_clr_mode, mask, ret = -EINVAL;
> >>>>
> >>>>  	if (!map || !chip->irq) {
> >>>>  		dev_err(chip->dev, "incorrect parameters\n");
> >>>>@@ -382,15 +382,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
> >>>>  	}
> >>>>
> >>>>  	/*
> >>>>-	 * irq_mode defines the way of clearing interrupt. it's read-clear by
> >>>>-	 * default.
> >>>>+	 * irq_clr_on_wr defines the way of clearing interrupt by
> >>>>+	 * read/write(0/1).  It's read-clear by default.
> >>>>  	 */
> >>>>  	mask =
> >>>>  	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
> >>>>  	    PM800_WAKEUP2_INT_MASK;
> >>>>
> >>>>-	data = PM800_WAKEUP2_INT_CLEAR;
> >>>>-	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> >>>>+	irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
> >>>>+		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
> >>>>+	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
> >>>
> >>>What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> >>>PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> >>>directly without all of this faffing about.
> >>>
> >>>   regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
> >>>
> >>
> >>Because "irq_clr_method" is of boolean type.
> >>And macros which you are referring to is,
> >>
> >>#define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
> >>#define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
> >>
> >>
> >>And also, I feel it is more cleaner approach with the current code as
> >>register definition and userflag are maintained separately.
> >
> >I see your point, although it's a shame we have to have this code in
> >its place.
> >
> >One thing I think you can do though is rid chip->irq_clr_method, just
> >use the one you already have in pdata.
> >
> 
> Looking at the current code,
> Yes, this can be done, but I have to do some more changes around it,
> to make code cleaner,
> 
> change the signature of
> 
> static int device_irq_init_800(struct pm80x_chip *chip)
> 
> TO
> 
> static int device_irq_init_800(struct pm80x_chip *chip, struct
> pm80x_platform_data *pdata)
> 
> 
> and then only use pdata->irq_clr_method.
> 
> 
> How do you want to get this inside? V6 version? or separate patch?
> 
> I have one more cleanup patch in the queue, which I am planning to
> submit today, if you are ok then I can submit along with that.

Ideally not.  Don't you save the 'struct device' into *chip?  You
should use that to fetch the pdata, like:

pdata = dev_get_platdata(chip->dev);

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

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

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

[Vaibhav Hiremath]

On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>> On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
>>>>>
>>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>>> (page 0) controls the method of clearing interrupt
>>>>>> status of 88pm800 family of devices;
>>>>>>
>>>>>>    0: clear on read
>>>>>>    1: clear on write
>>>>>>
>>>>>> If pdata is not coming from board file, then set the
>>>>>> default irq clear method to "irq clear on write"
>>>>>>
>>>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>>>> to appropriate name.
>>>>>>
>>>>>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>> ---
>>>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>>>   include/linux/mfd/88pm80x.h | 10 ++++++++--
>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>> index d495737..66347be 100644
>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>> @@ -374,7 +374,7 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>>>>>   {
>>>>>>   	struct regmap *map = chip->regmap;
>>>>>>   	unsigned long flags = IRQF_ONESHOT;
>>>>>> -	int data, mask, ret = -EINVAL;
>>>>>> +	int irq_clr_mode, mask, ret = -EINVAL;
>>>>>>
>>>>>>   	if (!map || !chip->irq) {
>>>>>>   		dev_err(chip->dev, "incorrect parameters\n");
>>>>>> @@ -382,15 +382,16 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>>>>>>   	}
>>>>>>
>>>>>>   	/*
>>>>>> -	 * irq_mode defines the way of clearing interrupt. it's read-clear by
>>>>>> -	 * default.
>>>>>> +	 * irq_clr_on_wr defines the way of clearing interrupt by
>>>>>> +	 * read/write(0/1).  It's read-clear by default.
>>>>>>   	 */
>>>>>>   	mask =
>>>>>>   	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>>>>>   	    PM800_WAKEUP2_INT_MASK;
>>>>>>
>>>>>> -	data = PM800_WAKEUP2_INT_CLEAR;
>>>>>> -	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>>>>>> +	irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
>>>>>> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;
>>>>>> +	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode);
>>>>>
>>>>> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
>>>>> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
>>>>> directly without all of this faffing about.
>>>>>
>>>>>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>>>>>
>>>>
>>>> Because "irq_clr_method" is of boolean type.
>>>> And macros which you are referring to is,
>>>>
>>>> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
>>>> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
>>>>
>>>>
>>>> And also, I feel it is more cleaner approach with the current code as
>>>> register definition and userflag are maintained separately.
>>>
>>> I see your point, although it's a shame we have to have this code in
>>> its place.
>>>
>>> One thing I think you can do though is rid chip->irq_clr_method, just
>>> use the one you already have in pdata.
>>>
>>
>> Looking at the current code,
>> Yes, this can be done, but I have to do some more changes around it,
>> to make code cleaner,
>>
>> change the signature of
>>
>> static int device_irq_init_800(struct pm80x_chip *chip)
>>
>> TO
>>
>> static int device_irq_init_800(struct pm80x_chip *chip, struct
>> pm80x_platform_data *pdata)
>>
>>
>> and then only use pdata->irq_clr_method.
>>
>>
>> How do you want to get this inside? V6 version? or separate patch?
>>
>> I have one more cleanup patch in the queue, which I am planning to
>> submit today, if you are ok then I can submit along with that.
>
> Ideally not.  Don't you save the 'struct device' into *chip?  You
> should use that to fetch the pdata, like:
>
> pdata = dev_get_platdata(chip->dev);
>

Yes certainly, this is another option (rather preferred one).

But to be consistent with other's I proposed this, please refer to the
fn device_800_init(), where all xxx_init() are taking 2 arguments, and
second argument is pdata.


There is room for cleanup, I agree.
I can put this too in the next cleanup series.

Thanks,
Vaibhav

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

[Vaibhav Hiremath]

On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
>
>
> On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>>> On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
>>>>>>
>>>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>>>> (page 0) controls the method of clearing interrupt
>>>>>>> status of 88pm800 family of devices;
>>>>>>>
>>>>>>>    0: clear on read
>>>>>>>    1: clear on write
>>>>>>>
>>>>>>> If pdata is not coming from board file, then set the
>>>>>>> default irq clear method to "irq clear on write"
>>>>>>>
>>>>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>>>>> to appropriate name.
>>>>>>>
>>>>>>> Signed-off-by: Zhao Ye <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 | 10 ++++++++--
>>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>>>>>>> index d495737..66347be 100644
>>>>>>> --- a/drivers/mfd/88pm800.c
>>>>>>> +++ b/drivers/mfd/88pm800.c
>>>>>>> @@ -374,7 +374,7 @@ static int device_irq_init_800(struct
>>>>>>> pm80x_chip *chip)
>>>>>>>   {
>>>>>>>       struct regmap *map = chip->regmap;
>>>>>>>       unsigned long flags = IRQF_ONESHOT;
>>>>>>> -    int data, mask, ret = -EINVAL;
>>>>>>> +    int irq_clr_mode, mask, ret = -EINVAL;
>>>>>>>
>>>>>>>       if (!map || !chip->irq) {
>>>>>>>           dev_err(chip->dev, "incorrect parameters\n");
>>>>>>> @@ -382,15 +382,16 @@ static int device_irq_init_800(struct
>>>>>>> pm80x_chip *chip)
>>>>>>>       }
>>>>>>>
>>>>>>>       /*
>>>>>>> -     * irq_mode defines the way of clearing interrupt. it's
>>>>>>> read-clear by
>>>>>>> -     * default.
>>>>>>> +     * irq_clr_on_wr defines the way of clearing interrupt by
>>>>>>> +     * read/write(0/1).  It's read-clear by default.
>>>>>>>        */
>>>>>>>       mask =
>>>>>>>           PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>>>>>>>           PM800_WAKEUP2_INT_MASK;
>>>>>>>
>>>>>>> -    data = PM800_WAKEUP2_INT_CLEAR;
>>>>>>> -    ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>>>>>>> +    irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
>>>>>>> +        PM800_WAKEUP2_INT_WRITE_CLEAR :
>>>>>>> PM800_WAKEUP2_INT_READ_CLEAR;
>>>>>>> +    ret = regmap_update_bits(map, PM800_WAKEUP2, mask,
>>>>>>> irq_clr_mode);
>>>>>>
>>>>>> What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
>>>>>> PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
>>>>>> directly without all of this faffing about.
>>>>>>
>>>>>>    regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
>>>>>>
>>>>>
>>>>> Because "irq_clr_method" is of boolean type.
>>>>> And macros which you are referring to is,
>>>>>
>>>>> #define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
>>>>> #define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
>>>>>
>>>>>
>>>>> And also, I feel it is more cleaner approach with the current code as
>>>>> register definition and userflag are maintained separately.
>>>>
>>>> I see your point, although it's a shame we have to have this code in
>>>> its place.
>>>>
>>>> One thing I think you can do though is rid chip->irq_clr_method, just
>>>> use the one you already have in pdata.
>>>>
>>>
>>> Looking at the current code,
>>> Yes, this can be done, but I have to do some more changes around it,
>>> to make code cleaner,
>>>
>>> change the signature of
>>>
>>> static int device_irq_init_800(struct pm80x_chip *chip)
>>>
>>> TO
>>>
>>> static int device_irq_init_800(struct pm80x_chip *chip, struct
>>> pm80x_platform_data *pdata)
>>>
>>>
>>> and then only use pdata->irq_clr_method.
>>>
>>>
>>> How do you want to get this inside? V6 version? or separate patch?
>>>
>>> I have one more cleanup patch in the queue, which I am planning to
>>> submit today, if you are ok then I can submit along with that.
>>
>> Ideally not.  Don't you save the 'struct device' into *chip?  You
>> should use that to fetch the pdata, like:
>>
>> pdata = dev_get_platdata(chip->dev);
>>
>
> Yes certainly, this is another option (rather preferred one).
>
> But to be consistent with other's I proposed this, please refer to the
> fn device_800_init(), where all xxx_init() are taking 2 arguments, and
> second argument is pdata.
>
>
> There is room for cleanup, I agree.
> I can put this too in the next cleanup series.
>

Note that this is init function, called from probe.

So both approach looks ok to me.

Thanks,
Vaibhav
--
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

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

[Lee Jones]

On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:

> 
> 
> On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
> >
> >
> >On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
> >>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> >>>>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>>>On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >>>>>>On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
> >>>>>>
> >>>>>>>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>>>>>>(page 0) controls the method of clearing interrupt
> >>>>>>>status of 88pm800 family of devices;
> >>>>>>>
> >>>>>>>   0: clear on read
> >>>>>>>   1: clear on write
> >>>>>>>
> >>>>>>>If pdata is not coming from board file, then set the
> >>>>>>>default irq clear method to "irq clear on write"
> >>>>>>>
> >>>>>>>Also, as suggested by "Lee Jones" renaming variable field
> >>>>>>>to appropriate name.
> >>>>>>>
> >>>>>>>Signed-off-by: Zhao Ye <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 | 10 ++++++++--
> >>>>>>>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>>>>>>
> >>>>>>>diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> >>>>>>>index d495737..66347be 100644
> >>>>>>>--- a/drivers/mfd/88pm800.c
> >>>>>>>+++ b/drivers/mfd/88pm800.c
> >>>>>>>@@ -374,7 +374,7 @@ static int device_irq_init_800(struct
> >>>>>>>pm80x_chip *chip)
> >>>>>>>  {
> >>>>>>>      struct regmap *map = chip->regmap;
> >>>>>>>      unsigned long flags = IRQF_ONESHOT;
> >>>>>>>-    int data, mask, ret = -EINVAL;
> >>>>>>>+    int irq_clr_mode, mask, ret = -EINVAL;
> >>>>>>>
> >>>>>>>      if (!map || !chip->irq) {
> >>>>>>>          dev_err(chip->dev, "incorrect parameters\n");
> >>>>>>>@@ -382,15 +382,16 @@ static int device_irq_init_800(struct
> >>>>>>>pm80x_chip *chip)
> >>>>>>>      }
> >>>>>>>
> >>>>>>>      /*
> >>>>>>>-     * irq_mode defines the way of clearing interrupt. it's
> >>>>>>>read-clear by
> >>>>>>>-     * default.
> >>>>>>>+     * irq_clr_on_wr defines the way of clearing interrupt by
> >>>>>>>+     * read/write(0/1).  It's read-clear by default.
> >>>>>>>       */
> >>>>>>>      mask =
> >>>>>>>          PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
> >>>>>>>          PM800_WAKEUP2_INT_MASK;
> >>>>>>>
> >>>>>>>-    data = PM800_WAKEUP2_INT_CLEAR;
> >>>>>>>-    ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
> >>>>>>>+    irq_clr_mode = chip->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ?
> >>>>>>>+        PM800_WAKEUP2_INT_WRITE_CLEAR :
> >>>>>>>PM800_WAKEUP2_INT_READ_CLEAR;
> >>>>>>>+    ret = regmap_update_bits(map, PM800_WAKEUP2, mask,
> >>>>>>>irq_clr_mode);
> >>>>>>
> >>>>>>What's stopping you just passing PM800_WAKEUP2_INT_WRITE_CLEAR or
> >>>>>>PM800_WAKEUP2_INT_READ_CLEAR from pdata?  Then you can use the value
> >>>>>>directly without all of this faffing about.
> >>>>>>
> >>>>>>   regmap_update_bits(map, PM800_WAKEUP2, mask, pdata->irq_clr_mode);
> >>>>>>
> >>>>>
> >>>>>Because "irq_clr_method" is of boolean type.
> >>>>>And macros which you are referring to is,
> >>>>>
> >>>>>#define PM800_WAKEUP2_INT_READ_CLEAR            (0 << 1)
> >>>>>#define PM800_WAKEUP2_INT_WRITE_CLEAR           (1 << 1)
> >>>>>
> >>>>>
> >>>>>And also, I feel it is more cleaner approach with the current code as
> >>>>>register definition and userflag are maintained separately.
> >>>>
> >>>>I see your point, although it's a shame we have to have this code in
> >>>>its place.
> >>>>
> >>>>One thing I think you can do though is rid chip->irq_clr_method, just
> >>>>use the one you already have in pdata.
> >>>>
> >>>
> >>>Looking at the current code,
> >>>Yes, this can be done, but I have to do some more changes around it,
> >>>to make code cleaner,
> >>>
> >>>change the signature of
> >>>
> >>>static int device_irq_init_800(struct pm80x_chip *chip)
> >>>
> >>>TO
> >>>
> >>>static int device_irq_init_800(struct pm80x_chip *chip, struct
> >>>pm80x_platform_data *pdata)
> >>>
> >>>
> >>>and then only use pdata->irq_clr_method.
> >>>
> >>>
> >>>How do you want to get this inside? V6 version? or separate patch?
> >>>
> >>>I have one more cleanup patch in the queue, which I am planning to
> >>>submit today, if you are ok then I can submit along with that.
> >>
> >>Ideally not.  Don't you save the 'struct device' into *chip?  You
> >>should use that to fetch the pdata, like:
> >>
> >>pdata = dev_get_platdata(chip->dev);
> >>
> >
> >Yes certainly, this is another option (rather preferred one).
> >
> >But to be consistent with other's I proposed this, please refer to the
> >fn device_800_init(), where all xxx_init() are taking 2 arguments, and
> >second argument is pdata.
> >
> >
> >There is room for cleanup, I agree.
> >I can put this too in the next cleanup series.
> >
> 
> Note that this is init function, called from probe.
> 
> So both approach looks ok to me.

Please clean up the other.  Function and put it at the front of the
set when you re-submit.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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

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

[Vaibhav Hiremath]

On Tuesday 07 July 2015 06:24 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>>>>> On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
>>>>>>>>
>>>>>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>>>>>> (page 0) controls the method of clearing interrupt
>>>>>>>>> status of 88pm800 family of devices;
>>>>>>>>>
>>>>>>>>>    0: clear on read
>>>>>>>>>    1: clear on write
>>>>>>>>>
>>>>>>>>> If pdata is not coming from board file, then set the
>>>>>>>>> default irq clear method to "irq clear on write"
>>>>>>>>>
>>>>>>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>>>>>>> to appropriate name.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>>>>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>>>>> ---
>>>>>>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>>>>>>   include/linux/mfd/88pm80x.h | 10 ++++++++--
>>>>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>>>>

<snip>

>>>
>>> Yes certainly, this is another option (rather preferred one).
>>>
>>> But to be consistent with other's I proposed this, please refer to the
>>> fn device_800_init(), where all xxx_init() are taking 2 arguments, and
>>> second argument is pdata.
>>>
>>>
>>> There is room for cleanup, I agree.
>>> I can put this too in the next cleanup series.
>>>
>>
>> Note that this is init function, called from probe.
>>
>> So both approach looks ok to me.
>
> Please clean up the other.  Function and put it at the front of the
> set when you re-submit.
>

Sorry for dumb question here :)
I did not understand what do you mean by "in front of the set"?

You want to see all the patches into one single series?
Or
have separate series,
	1. existing DT addition series
	2. new clean-up series

Thanks,
Vaibhav

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

[Lee Jones]

On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:

> 
> 
> On Tuesday 07 July 2015 06:24 PM, Lee Jones wrote:
> >On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >
> >>
> >>
> >>On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
> >>>
> >>>
> >>>On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
> >>>>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>>>On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
> >>>>>>On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
> >>>>>>>On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
> >>>>>>>>On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
> >>>>>>>>
> >>>>>>>>>As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
> >>>>>>>>>(page 0) controls the method of clearing interrupt
> >>>>>>>>>status of 88pm800 family of devices;
> >>>>>>>>>
> >>>>>>>>>   0: clear on read
> >>>>>>>>>   1: clear on write
> >>>>>>>>>
> >>>>>>>>>If pdata is not coming from board file, then set the
> >>>>>>>>>default irq clear method to "irq clear on write"
> >>>>>>>>>
> >>>>>>>>>Also, as suggested by "Lee Jones" renaming variable field
> >>>>>>>>>to appropriate name.
> >>>>>>>>>
> >>>>>>>>>Signed-off-by: Zhao Ye <zhaoy@marvell.com>
> >>>>>>>>>Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> >>>>>>>>>---
> >>>>>>>>>  drivers/mfd/88pm800.c       | 15 ++++++++++-----
> >>>>>>>>>  include/linux/mfd/88pm80x.h | 10 ++++++++--
> >>>>>>>>>  2 files changed, 18 insertions(+), 7 deletions(-)
> >>>>>>>>>
> 
> <snip>
> 
> >>>
> >>>Yes certainly, this is another option (rather preferred one).
> >>>
> >>>But to be consistent with other's I proposed this, please refer to the
> >>>fn device_800_init(), where all xxx_init() are taking 2 arguments, and
> >>>second argument is pdata.
> >>>
> >>>
> >>>There is room for cleanup, I agree.
> >>>I can put this too in the next cleanup series.
> >>>
> >>
> >>Note that this is init function, called from probe.
> >>
> >>So both approach looks ok to me.
> >
> >Please clean up the other.  Function and put it at the front of the
> >set when you re-submit.
> >
> 
> Sorry for dumb question here :)
> I did not understand what do you mean by "in front of the set"?
> 
> You want to see all the patches into one single series?
> Or
> have separate series,
> 	1. existing DT addition series
> 	2. new clean-up series

[PATCH v6 0/5] mfd: 88pm800: Add Device tree support
├>[PATCH v6 1/5] mfd: 88pm800: Obtain pdata from 'device' rather than passing as parameter
├>[PATCH v6 2/5] mfd: 88pm800: Add device tree support
├>[PATCH v6 3/5] mfd: 88pm800: Remove unnecessary protection around pdata
├>[PATCH v6 4/5] mfd: 88pm800: Set default interrupt clear method
├>[PATCH v6 5/5] mfd: devicetree: bindings: Add new 88pm800 mfd binding

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

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

[Vaibhav Hiremath]

On Tuesday 07 July 2015 08:27 PM, Lee Jones wrote:
> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>
>>
>>
>> On Tuesday 07 July 2015 06:24 PM, Lee Jones wrote:
>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>
>>>>
>>>>
>>>> On Tuesday 07 July 2015 04:48 PM, Vaibhav Hiremath wrote:
>>>>>
>>>>>
>>>>> On Tuesday 07 July 2015 04:42 PM, Lee Jones wrote:
>>>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>>>> On Tuesday 07 July 2015 04:10 PM, Lee Jones wrote:
>>>>>>>> On Tue, 07 Jul 2015, Vaibhav Hiremath wrote:
>>>>>>>>> On Tuesday 07 July 2015 12:59 PM, Lee Jones wrote:
>>>>>>>>>> On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:
>>>>>>>>>>
>>>>>>>>>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe
>>>>>>>>>>> (page 0) controls the method of clearing interrupt
>>>>>>>>>>> status of 88pm800 family of devices;
>>>>>>>>>>>
>>>>>>>>>>>    0: clear on read
>>>>>>>>>>>    1: clear on write
>>>>>>>>>>>
>>>>>>>>>>> If pdata is not coming from board file, then set the
>>>>>>>>>>> default irq clear method to "irq clear on write"
>>>>>>>>>>>
>>>>>>>>>>> Also, as suggested by "Lee Jones" renaming variable field
>>>>>>>>>>> to appropriate name.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Zhao Ye <zhaoy@marvell.com>
>>>>>>>>>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>>   drivers/mfd/88pm800.c       | 15 ++++++++++-----
>>>>>>>>>>>   include/linux/mfd/88pm80x.h | 10 ++++++++--
>>>>>>>>>>>   2 files changed, 18 insertions(+), 7 deletions(-)
>>>>>>>>>>>
>>
>> <snip>
>>
>>>>>
>>>>> Yes certainly, this is another option (rather preferred one).
>>>>>
>>>>> But to be consistent with other's I proposed this, please refer to the
>>>>> fn device_800_init(), where all xxx_init() are taking 2 arguments, and
>>>>> second argument is pdata.
>>>>>
>>>>>
>>>>> There is room for cleanup, I agree.
>>>>> I can put this too in the next cleanup series.
>>>>>
>>>>
>>>> Note that this is init function, called from probe.
>>>>
>>>> So both approach looks ok to me.
>>>
>>> Please clean up the other.  Function and put it at the front of the
>>> set when you re-submit.
>>>
>>
>> Sorry for dumb question here :)
>> I did not understand what do you mean by "in front of the set"?
>>
>> You want to see all the patches into one single series?
>> Or
>> have separate series,
>> 	1. existing DT addition series
>> 	2. new clean-up series
>
> [PATCH v6 0/5] mfd: 88pm800: Add Device tree support
> ├>[PATCH v6 1/5] mfd: 88pm800: Obtain pdata from 'device' rather than passing as parameter
> ├>[PATCH v6 2/5] mfd: 88pm800: Add device tree support
> ├>[PATCH v6 3/5] mfd: 88pm800: Remove unnecessary protection around pdata
> ├>[PATCH v6 4/5] mfd: 88pm800: Set default interrupt clear method
> ├>[PATCH v6 5/5] mfd: devicetree: bindings: Add new 88pm800 mfd binding
>

Thanks Lee,

Just FYI,
I have done some reordering here, because of obvious reasons.

Submitting patches shortly...

Thanks,
Vaibhav

[PATCH-V5 4/4] 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 | 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

--
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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Krzysztof Kozłowski]

2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> 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>

Looks okay,

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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Lee Jones]

On Tue, 30 Jun 2015, Krzysztof Kozłowski wrote:

> 2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
> > 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>
> 
> Looks okay,

You forgot to add your *-by.

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

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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Krzysztof Kozłowski]

2015-07-01 16:43 GMT+09:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 30 Jun 2015, Krzysztof Kozłowski wrote:
>
>> 2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>> > 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>
>>
>> Looks okay,
>
> You forgot to add your *-by.

Yeah, I hit the "enter" too early. I sent later another email. Sorry
for the noise.

Best regards,
Krzysztof

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Krzysztof Kozlowski]

2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> 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>

(once again, too early hit return)

Looks okay. Just a hint - put the binding at beginning of the patchset.

Reviewed-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Tuesday 30 June 2015 06:01 AM, Krzysztof Kozlowski wrote:
> 2015-06-30 0:31 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
>> 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>
>
> (once again, too early hit return)
>
> Looks okay. Just a hint - put the binding at beginning of the patchset.
>

OK, next time onwards will take care of it.

> Reviewed-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>


Thanks for your review.


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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Lee Jones]

On Mon, 29 Jun 2015, Vaibhav Hiremath wrote:

> With addition of DT support to 88pm800 mfd driver, this patch
> adds new DT binding documentation along with respective properties.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 53 +++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt

For my own reference:
  Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yes, I plan to take this through the MFD tree.

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Rob Herring]

On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> 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>

Didn't I ack this already? You should add it when you send subsequent
versions. The maintainer will add it if there aren't any subsequent
versions.

Acked-by: Rob Herring <robh@kernel.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-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Wednesday 01 July 2015 09:40 PM, Rob Herring wrote:
> On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> 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>
>
> Didn't I ack this already? You should add it when you send subsequent
> versions. The maintainer will add it if there aren't any subsequent
> versions.
>
> Acked-by: Rob Herring <robh@kernel.org>
>

Sorry Rob. Infact I did ask you as well. Probably you missed it.

Patch got changed after your ack, I did some changes based on few
review comments, from Lee and others.

Anyway, thanks for ack. Can you please queue this?

Thanks,
Vaibhav

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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Rob Herring]

On Wed, Jul 1, 2015 at 11:21 AM, Vaibhav Hiremath
<vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>
> On Wednesday 01 July 2015 09:40 PM, Rob Herring wrote:
>>
>> On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
>> <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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>
>>
>>
>> Didn't I ack this already? You should add it when you send subsequent
>> versions. The maintainer will add it if there aren't any subsequent
>> versions.
>>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>
>
> Sorry Rob. Infact I did ask you as well. Probably you missed it.
>
> Patch got changed after your ack, I did some changes based on few
> review comments, from Lee and others.

Ok, then it is up to your judgement whether the ack should still
apply. If not, you should note that.

> Anyway, thanks for ack. Can you please queue this?

I'm assuming Lee is taking the whole series.

Rob
--
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

Re: [PATCH-V5 4/4] mfd: devicetree: bindings: Add new 88pm800 mfd binding

[Vaibhav Hiremath]

On Wednesday 01 July 2015 10:47 PM, Rob Herring wrote:
> On Wed, Jul 1, 2015 at 11:21 AM, Vaibhav Hiremath
> <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>
>> On Wednesday 01 July 2015 09:40 PM, Rob Herring wrote:
>>>
>>> On Mon, Jun 29, 2015 at 10:31 AM, Vaibhav Hiremath
>>> <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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>
>>>
>>>
>>> Didn't I ack this already? You should add it when you send subsequent
>>> versions. The maintainer will add it if there aren't any subsequent
>>> versions.
>>>
>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>
>>
>> Sorry Rob. Infact I did ask you as well. Probably you missed it.
>>
>> Patch got changed after your ack, I did some changes based on few
>> review comments, from Lee and others.
>
> Ok, then it is up to your judgement whether the ack should still
> apply. If not, you should note that.
>

I did.

I did mention it in my cover letter.

>> Anyway, thanks for ack. Can you please queue this?
>
> I'm assuming Lee is taking the whole series.
>

Ok.

Thanks,
Vaibhav
--
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