[PATCH RESEND] max77693: added device tree support

[PATCH RESEND] max77693: added device tree support

[Andrzej Hajda]

max77693 mfd main device uses only wakeup field
from max77693_platform_data. This field is mapped
to wakeup-source common property in device tree.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Hi Lee,

This is the patch for max77693 DT support rebased onto v3.11-rc5.

Regards
Andrzej
---

 Documentation/devicetree/bindings/mfd/max77693.txt |  2 +
 drivers/mfd/max77693.c                             | 50 ++++++++++++++++------
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
index 11921cc..4138ad4 100644
--- a/Documentation/devicetree/bindings/mfd/max77693.txt
+++ b/Documentation/devicetree/bindings/mfd/max77693.txt
@@ -19,6 +19,8 @@ Required properties:
 Optional properties:
 - regulators : The regulators of max77693 have to be instantiated under subnod
   named "regulators" using the following format.
+- wakeup-source : Indicates if the device can wakeup the system from the sleep
+  state.
 
 	regulators {
 		regualtor-compatible = ESAFEOUT1/ESAFEOUT2/CHARGER
diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
index 9e60fed..6045706 100644
--- a/drivers/mfd/max77693.c
+++ b/drivers/mfd/max77693.c
@@ -106,29 +106,41 @@ static const struct regmap_config max77693_regmap_config = {
 	.max_register = MAX77693_PMIC_REG_END,
 };
 
+static int max77693_get_platform_data(struct max77693_dev *max77693,
+				      struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	struct max77693_platform_data *pdata = dev->platform_data;
+
+	if (node) {
+		max77693->wakeup = of_property_read_bool(node, "wakeup-source");
+		return 0;
+	}
+
+	if (pdata) {
+		max77693->wakeup = pdata->wakeup;
+		return 0;
+	}
+
+	dev_err(dev, "No platform data found.\n");
+	return -EINVAL;
+}
+
 static int max77693_i2c_probe(struct i2c_client *i2c,
 			      const struct i2c_device_id *id)
 {
 	struct max77693_dev *max77693;
-	struct max77693_platform_data *pdata = i2c->dev.platform_data;
 	u8 reg_data;
 	int ret = 0;
 
-	if (!pdata) {
-		dev_err(&i2c->dev, "No platform data found.\n");
-		return -EINVAL;
-	}
-
 	max77693 = devm_kzalloc(&i2c->dev,
 			sizeof(struct max77693_dev), GFP_KERNEL);
 	if (max77693 == NULL)
 		return -ENOMEM;
 
-	i2c_set_clientdata(i2c, max77693);
-	max77693->dev = &i2c->dev;
-	max77693->i2c = i2c;
-	max77693->irq = i2c->irq;
-	max77693->type = id->driver_data;
+	ret = max77693_get_platform_data(max77693, &i2c->dev);
+	if (ret < 0)
+		return ret;
 
 	max77693->regmap = devm_regmap_init_i2c(i2c, &max77693_regmap_config);
 	if (IS_ERR(max77693->regmap)) {
@@ -138,7 +150,11 @@ static int max77693_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	max77693->wakeup = pdata->wakeup;
+	i2c_set_clientdata(i2c, max77693);
+	max77693->dev = &i2c->dev;
+	max77693->i2c = i2c;
+	max77693->irq = i2c->irq;
+	max77693->type = id->driver_data;
 
 	ret = max77693_read_reg(max77693->regmap, MAX77693_PMIC_REG_PMIC_ID2,
 				&reg_data);
@@ -179,7 +195,7 @@ static int max77693_i2c_probe(struct i2c_client *i2c,
 	if (ret < 0)
 		goto err_mfd;
 
-	device_init_wakeup(max77693->dev, pdata->wakeup);
+	device_init_wakeup(max77693->dev, max77693->wakeup);
 
 	return ret;
 
@@ -235,11 +251,19 @@ static const struct dev_pm_ops max77693_pm = {
 	.resume = max77693_resume,
 };
 
+#ifdef CONFIG_OF
+static struct of_device_id max77693_dt_match[] = {
+	{.compatible = "maxim,max77693"},
+	{},
+};
+#endif
+
 static struct i2c_driver max77693_i2c_driver = {
 	.driver = {
 		   .name = "max77693",
 		   .owner = THIS_MODULE,
 		   .pm = &max77693_pm,
+		   .of_match_table = of_match_ptr(max77693_dt_match),
 	},
 	.probe = max77693_i2c_probe,
 	.remove = max77693_i2c_remove,
-- 
1.8.1.2

Re: [PATCH RESEND] max77693: added device tree support

[Lee Jones]

> +static int max77693_get_platform_data(struct max77693_dev *max77693,
> +				      struct device *dev)
> +{
> +	struct device_node *node = dev->of_node;
> +	struct max77693_platform_data *pdata = dev->platform_data;
> +
> +	if (node) {
> +		max77693->wakeup = of_property_read_bool(node, "wakeup-source");
> +		return 0;
> +	}
> +
> +	if (pdata) {
> +		max77693->wakeup = pdata->wakeup;
> +		return 0;
> +	}

Hmm... Unless things have changed, pdata usually takes precedence over
DT i.e. if pdata is present it should be used. Can you reverse these
two if statements please? 

Sorry to mess you around.

> +	dev_err(dev, "No platform data found.\n");
> +	return -EINVAL;
> +}
> +

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

[PATCH v2] max77693: added device tree support

[Andrzej Hajda]

max77693 mfd main device uses only wakeup field
from max77693_platform_data. This field is mapped
to wakeup-source common property in device tree.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Hi Lee,

Thanks for the review.
Patch corrected according to your suggestion.

Regards
Andrzej
---
v2:
  - platform data takes precedence over DT
---
 Documentation/devicetree/bindings/mfd/max77693.txt |  2 +
 drivers/mfd/max77693.c                             | 50 ++++++++++++++++------
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
index 11921cc..4138ad4 100644
--- a/Documentation/devicetree/bindings/mfd/max77693.txt
+++ b/Documentation/devicetree/bindings/mfd/max77693.txt
@@ -19,6 +19,8 @@ Required properties:
 Optional properties:
 - regulators : The regulators of max77693 have to be instantiated under subnod
   named "regulators" using the following format.
+- wakeup-source : Indicates if the device can wakeup the system from the sleep
+  state.
 
 	regulators {
 		regualtor-compatible = ESAFEOUT1/ESAFEOUT2/CHARGER
diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
index 9e60fed..d85663d 100644
--- a/drivers/mfd/max77693.c
+++ b/drivers/mfd/max77693.c
@@ -106,29 +106,41 @@ static const struct regmap_config max77693_regmap_config = {
 	.max_register = MAX77693_PMIC_REG_END,
 };
 
+static int max77693_get_platform_data(struct max77693_dev *max77693,
+				      struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	struct max77693_platform_data *pdata = dev->platform_data;
+
+	if (pdata) {
+		max77693->wakeup = pdata->wakeup;
+		return 0;
+	}
+
+	if (node) {
+		max77693->wakeup = of_property_read_bool(node, "wakeup-source");
+		return 0;
+	}
+
+	dev_err(dev, "No platform data found.\n");
+	return -EINVAL;
+}
+
 static int max77693_i2c_probe(struct i2c_client *i2c,
 			      const struct i2c_device_id *id)
 {
 	struct max77693_dev *max77693;
-	struct max77693_platform_data *pdata = i2c->dev.platform_data;
 	u8 reg_data;
 	int ret = 0;
 
-	if (!pdata) {
-		dev_err(&i2c->dev, "No platform data found.\n");
-		return -EINVAL;
-	}
-
 	max77693 = devm_kzalloc(&i2c->dev,
 			sizeof(struct max77693_dev), GFP_KERNEL);
 	if (max77693 == NULL)
 		return -ENOMEM;
 
-	i2c_set_clientdata(i2c, max77693);
-	max77693->dev = &i2c->dev;
-	max77693->i2c = i2c;
-	max77693->irq = i2c->irq;
-	max77693->type = id->driver_data;
+	ret = max77693_get_platform_data(max77693, &i2c->dev);
+	if (ret < 0)
+		return ret;
 
 	max77693->regmap = devm_regmap_init_i2c(i2c, &max77693_regmap_config);
 	if (IS_ERR(max77693->regmap)) {
@@ -138,7 +150,11 @@ static int max77693_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	max77693->wakeup = pdata->wakeup;
+	i2c_set_clientdata(i2c, max77693);
+	max77693->dev = &i2c->dev;
+	max77693->i2c = i2c;
+	max77693->irq = i2c->irq;
+	max77693->type = id->driver_data;
 
 	ret = max77693_read_reg(max77693->regmap, MAX77693_PMIC_REG_PMIC_ID2,
 				&reg_data);
@@ -179,7 +195,7 @@ static int max77693_i2c_probe(struct i2c_client *i2c,
 	if (ret < 0)
 		goto err_mfd;
 
-	device_init_wakeup(max77693->dev, pdata->wakeup);
+	device_init_wakeup(max77693->dev, max77693->wakeup);
 
 	return ret;
 
@@ -235,11 +251,19 @@ static const struct dev_pm_ops max77693_pm = {
 	.resume = max77693_resume,
 };
 
+#ifdef CONFIG_OF
+static struct of_device_id max77693_dt_match[] = {
+	{.compatible = "maxim,max77693"},
+	{},
+};
+#endif
+
 static struct i2c_driver max77693_i2c_driver = {
 	.driver = {
 		   .name = "max77693",
 		   .owner = THIS_MODULE,
 		   .pm = &max77693_pm,
+		   .of_match_table = of_match_ptr(max77693_dt_match),
 	},
 	.probe = max77693_i2c_probe,
 	.remove = max77693_i2c_remove,
-- 
1.8.1.2

Re: [PATCH v2] max77693: added device tree support

[Lee Jones]

> max77693 mfd main device uses only wakeup field
> from max77693_platform_data. This field is mapped
> to wakeup-source common property in device tree.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> ---
>  Documentation/devicetree/bindings/mfd/max77693.txt |  2 +
>  drivers/mfd/max77693.c                             | 50 ++++++++++++++++------
>  2 files changed, 39 insertions(+), 13 deletions(-)

Well, I know this patch has been sat on the MLs for some time now, so
anyone who wanted to have a say have has plenty of opportunity and the
patch looks good to me now, so:

Applied, thanks.

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

Re: [PATCH v2] max77693: added device tree support

[Stephen Warren]

On 08/19/2013 05:40 AM, Andrzej Hajda wrote:
> max77693 mfd main device uses only wakeup field
> from max77693_platform_data. This field is mapped
> to wakeup-source common property in device tree.

> diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt

>  Optional properties:
>  - regulators : The regulators of max77693 have to be instantiated under subnod
>    named "regulators" using the following format.
> +- wakeup-source : Indicates if the device can wakeup the system from the sleep
> +  state.

Does the property mean "can" or "should"?

"Can" implies that the property means something about the HW. What
exactly does it mean; perhaps that some specific output pin of the chip
has been wired to an input IRQ/GPIO of the SoC or PMIC that (can) wake
up the system? If so, which pin, signal, ...? Also, doesn't this also
depend on the SoC itself supporting its input IRQ/GPIO as a wakeup
source, so isn't some co-ordination required between the SoC and chip,
such that this property doesn't mean "can wakeup the system", but simply
"a signal is routed to the SoC, so perhaps it can wakeup the system".

"Should" implies policy, which probably shouldn't be represented in
device tree, since DT should describe the HW and not how it should be used.

Finally, if there was already a binding for max77693.txt, I don't think
the patch subject "added device tree support" is entirely accurate; this
change to the binding document seems to be more about adding a new
feature than adding DT support to the driver...

Re: [PATCH v2] max77693: added device tree support

[Lee Jones]

On Mon, 19 Aug 2013, Stephen Warren wrote:

> On 08/19/2013 05:40 AM, Andrzej Hajda wrote:
> > max77693 mfd main device uses only wakeup field
> > from max77693_platform_data. This field is mapped
> > to wakeup-source common property in device tree.
> 
> > diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
> 
> >  Optional properties:
> >  - regulators : The regulators of max77693 have to be instantiated under subnod
> >    named "regulators" using the following format.
> > +- wakeup-source : Indicates if the device can wakeup the system from the sleep
> > +  state.
> 
> Does the property mean "can" or "should"?
> 
> "Can" implies that the property means something about the HW. What
> exactly does it mean; perhaps that some specific output pin of the chip
> has been wired to an input IRQ/GPIO of the SoC or PMIC that (can) wake
> up the system? If so, which pin, signal, ...? Also, doesn't this also
> depend on the SoC itself supporting its input IRQ/GPIO as a wakeup
> source, so isn't some co-ordination required between the SoC and chip,
> such that this property doesn't mean "can wakeup the system", but simply
> "a signal is routed to the SoC, so perhaps it can wakeup the system".
> 
> "Should" implies policy, which probably shouldn't be represented in
> device tree, since DT should describe the HW and not how it should be used.
> 
> Finally, if there was already a binding for max77693.txt, I don't think
> the patch subject "added device tree support" is entirely accurate; this
> change to the binding document seems to be more about adding a new
> feature than adding DT support to the driver...

I'm taking this as a NACK.

Once Stephen is happy I'll reapply any RESEND with his Ack.

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

Re: [PATCH v2] max77693: added device tree support

[Andrzej Hajda]

Hi,

Thanks for the review.

On 08/19/2013 11:18 PM, Stephen Warren wrote:
> On 08/19/2013 05:40 AM, Andrzej Hajda wrote:
>> max77693 mfd main device uses only wakeup field
>> from max77693_platform_data. This field is mapped
>> to wakeup-source common property in device tree.
>> diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
>>  Optional properties:
>>  - regulators : The regulators of max77693 have to be instantiated under subnod
>>    named "regulators" using the following format.
>> +- wakeup-source : Indicates if the device can wakeup the system from the sleep
>> +  state.
> Does the property mean "can" or "should"?
>
> "Can" implies that the property means something about the HW. What
> exactly does it mean; perhaps that some specific output pin of the chip
> has been wired to an input IRQ/GPIO of the SoC or PMIC that (can) wake
> up the system? If so, which pin, signal, ...? Also, doesn't this also
> depend on the SoC itself supporting its input IRQ/GPIO as a wakeup
> source, so isn't some co-ordination required between the SoC and chip,
> such that this property doesn't mean "can wakeup the system", but simply
> "a signal is routed to the SoC, so perhaps it can wakeup the system".
>
> "Should" implies policy, which probably shouldn't be represented in
> device tree, since DT should describe the HW and not how it should be used.
After short digging I have realized that "wakeup-source" property is
already parsed by 
of_i2c_register_devices core function and it should not be parsed again
by the driver itself.
So I suppose the description of it can be removed from max77693 binding.
Do you agree?

Anyway I will prepare separate patch removing wakeup related code from
probe and
related field from max77693 platform_data - i2c_board_info::flag field
should be used instead.
>
> Finally, if there was already a binding for max77693.txt, I don't think
> the patch subject "added device tree support" is entirely accurate; this
> change to the binding document seems to be more about adding a new
> feature than adding DT support to the driver...
>
This patch really adds DT support to max77693 mfd driver :)

The binding file which is already in kernel currently has no related
code in the max77693 driver.
I will add clarification in the commit message.

Regards
Andrzej

Re: [PATCH v2] max77693: added device tree support

[Stephen Warren]

On 08/20/2013 06:41 AM, Andrzej Hajda wrote:
> Hi,
> 
> Thanks for the review.
> 
> On 08/19/2013 11:18 PM, Stephen Warren wrote:
>> On 08/19/2013 05:40 AM, Andrzej Hajda wrote:
>>> max77693 mfd main device uses only wakeup field
>>> from max77693_platform_data. This field is mapped
>>> to wakeup-source common property in device tree.
>>> diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
>>>  Optional properties:
>>>  - regulators : The regulators of max77693 have to be instantiated under subnod
>>>    named "regulators" using the following format.
>>> +- wakeup-source : Indicates if the device can wakeup the system from the sleep
>>> +  state.
>>
>> Does the property mean "can" or "should"?
>>
>> "Can" implies that the property means something about the HW. What
>> exactly does it mean; perhaps that some specific output pin of the chip
>> has been wired to an input IRQ/GPIO of the SoC or PMIC that (can) wake
>> up the system? If so, which pin, signal, ...? Also, doesn't this also
>> depend on the SoC itself supporting its input IRQ/GPIO as a wakeup
>> source, so isn't some co-ordination required between the SoC and chip,
>> such that this property doesn't mean "can wakeup the system", but simply
>> "a signal is routed to the SoC, so perhaps it can wakeup the system".
>>
>> "Should" implies policy, which probably shouldn't be represented in
>> device tree, since DT should describe the HW and not how it should be used.
>
> After short digging I have realized that "wakeup-source" property is
> already parsed by 
> of_i2c_register_devices core function and it should not be parsed again
> by the driver itself.
> So I suppose the description of it can be removed from max77693 binding.
> Do you agree?

Well, that in turn means that the core I2C binding should describe this
property, and it doesn't. So, that just means you need to apply this
part of the patch to a different document instead, but doesn't remove
the need to document exactly what it means...

But, it's reasonable to go ahead and add DT support to the driver based
on the current binding in this patch, and add the documentation for this
property to the I2C core binding document in some other patch.