[PATCH 0/4] mfd: max8998: Device Tree support fixes

[PATCH 0/4] mfd: max8998: Device Tree support fixes

[Paweł Chmiel]

This patch series compose of 4 patches.

First patch, fixes platform data retrieval issue in max8998 regulator
driver, which could cause NULL pointer dereference.

Second patch, fixes platform data retrieval issue in max8998 charger
driver, which could cause NULL pointer dereference.

Third patch, updates max8998 charger driver, so it's possible to parse
devicetree for configuration.

Fourth patch, updates max8998 documentation, so it includes new node 
and properties, needed for charger.

All patches has been tested on (not yet mainlined), an S5PV210 based
Samsung Galaxy S (i9000) phone.

Paweł Chmiel (3):
  regulator: max8998: Fix platform data retrieval.
  power: supply: max8998-charger: Parse device tree for required data.
  dt-bindings: mfd: max8998: Add charger subnode binding

Tomasz Figa (1):
  power: supply: max8998-charger: Fix platform data retrieval

 Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++
 drivers/power/supply/max8998_charger.c            | 52 ++++++++++++++++++++++-
 drivers/regulator/max8998.c                       |  3 +-
 3 files changed, 74 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/4] regulator: max8998: Fix platform data retrieval.

[Paweł Chmiel]

Since the max8998 MFD driver supports instantiation by DT, platform data
retrieval is handled in MFD probe and cell drivers should get use
the pdata field of max8998_dev struct to obtain them.

Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/regulator/max8998.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/regulator/max8998.c b/drivers/regulator/max8998.c
index 3027e7ce100b..6a2b61c012b5 100644
--- a/drivers/regulator/max8998.c
+++ b/drivers/regulator/max8998.c
@@ -309,8 +309,7 @@ static int max8998_set_voltage_buck_sel(struct regulator_dev *rdev,
 					unsigned selector)
 {
 	struct max8998_data *max8998 = rdev_get_drvdata(rdev);
-	struct max8998_platform_data *pdata =
-		dev_get_platdata(max8998->iodev->dev);
+	struct max8998_platform_data *pdata = max8998->iodev->pdata;
 	struct i2c_client *i2c = max8998->iodev->i2c;
 	int buck = rdev_get_id(rdev);
 	int reg, shift = 0, mask, ret, j;
-- 
2.7.4

[PATCH 2/4] power: supply: max8998-charger: Fix platform data retrieval

[Paweł Chmiel]

From: Tomasz Figa <tomasz.figa@gmail.com>

Since the max8998 MFD driver supports instantiation by DT, platform data
retrieval is handled in MFD probe and cell drivers should get use
the pdata field of max8998_dev struct to obtain them.

Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/power/supply/max8998_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/max8998_charger.c b/drivers/power/supply/max8998_charger.c
index b64cf0f14142..66438029bdd0 100644
--- a/drivers/power/supply/max8998_charger.c
+++ b/drivers/power/supply/max8998_charger.c
@@ -85,7 +85,7 @@ static const struct power_supply_desc max8998_battery_desc = {
 static int max8998_battery_probe(struct platform_device *pdev)
 {
 	struct max8998_dev *iodev = dev_get_drvdata(pdev->dev.parent);
-	struct max8998_platform_data *pdata = dev_get_platdata(iodev->dev);
+	struct max8998_platform_data *pdata = iodev->pdata;
 	struct power_supply_config psy_cfg = {};
 	struct max8998_battery_data *max8998;
 	struct i2c_client *i2c;
-- 
2.7.4

[PATCH 3/4] power: supply: max8998-charger: Parse device tree for required data.

[Paweł Chmiel]

This patch adds missing code for reading charger configuration
from devicetree.

Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/power/supply/max8998_charger.c | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/power/supply/max8998_charger.c b/drivers/power/supply/max8998_charger.c
index 66438029bdd0..8fa0c3357625 100644
--- a/drivers/power/supply/max8998_charger.c
+++ b/drivers/power/supply/max8998_charger.c
@@ -21,6 +21,7 @@
 
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
@@ -82,6 +83,49 @@ static const struct power_supply_desc max8998_battery_desc = {
 	.num_properties	= ARRAY_SIZE(max8998_battery_props),
 };
 
+static int max8998_pmic_dt_parse_pdata(struct max8998_dev *iodev,
+					struct max8998_platform_data *pdata)
+{
+	struct device_node *pmic_np = iodev->dev->of_node;
+	struct device_node *charger_np;
+	int ret;
+
+	charger_np = of_get_child_by_name(pmic_np, "charger");
+	if (!charger_np) {
+		dev_err(iodev->dev, "could not find charger sub-node\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(charger_np,
+					"max8998,charge-eoc",
+					&pdata->eoc);
+	if (ret < 0) {
+		dev_err(iodev->dev,
+			"Could not find max8998,charge-eoc in devicetree\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(charger_np,
+					"max8998,charge-restart",
+					&pdata->restart);
+	if (ret < 0) {
+		dev_err(iodev->dev,
+			"Could not find max8998,charge-restart in devicetree\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(charger_np,
+					"max8998,charge-timeout",
+					&pdata->timeout);
+	if (ret < 0) {
+		dev_err(iodev->dev,
+			"Could not find max8998,charge-timeout in devicetree\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int max8998_battery_probe(struct platform_device *pdev)
 {
 	struct max8998_dev *iodev = dev_get_drvdata(pdev->dev.parent);
@@ -96,6 +140,12 @@ static int max8998_battery_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	if (IS_ENABLED(CONFIG_OF) && iodev->dev->of_node) {
+		ret = max8998_pmic_dt_parse_pdata(iodev, pdata);
+		if (ret)
+			return ret;
+	}
+
 	max8998 = devm_kzalloc(&pdev->dev, sizeof(struct max8998_battery_data),
 				GFP_KERNEL);
 	if (!max8998)
-- 
2.7.4

[PATCH 4/4] dt-bindings: mfd: max8998: Add charger subnode binding

[Paweł Chmiel]

This patch adds devicetree bindings documentation for
battery charging controller as the subnode of MAX8998 PMIC.
It's based on current behavior of driver.

Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
index 23a3650ff2a2..f95610afb57f 100644
--- a/Documentation/devicetree/bindings/mfd/max8998.txt
+++ b/Documentation/devicetree/bindings/mfd/max8998.txt
@@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
 - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
   for buck2 regulator that can be selected using dvs gpio.
 
+Charger: Configuration for battery charging controller should be added
+inside a child node named 'charger'.
+  Required properties:
+  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
+    remain value set from bootloader or default value will be used.
+    Valid values: 0, 10 - 45
+
+  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
+    remain value set from bootloader or default value will be used.
+    Valid values: -1, 0, 100, 150, 200
+
+  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
+    remain value set from bootloader or default value will be used.
+    Valid values: -1, 0, 5, 6, 7
+
 Regulators: All the regulators of MAX8998 to be instantiated shall be
 listed in a child node named 'regulators'. Each regulator is represented
 by a child node of the 'regulators' node.
@@ -99,6 +114,13 @@ Example:
 		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
 		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
 
+		/* Charger configuration */
+		charger {
+			max8998,charge-eoc = <0>;
+			max8998,charge-restart = <(-1)>;
+			max8998,charge-timeout = <7>;
+		};
+
 		/* Regulators to instantiate */
 		regulators {
 			ldo2_reg: LDO2 {
-- 
2.7.4

Re: [PATCH 4/4] dt-bindings: mfd: max8998: Add charger subnode binding

[Lee Jones]

On Fri, 27 Apr 2018, Paweł Chmiel wrote:

> This patch adds devicetree bindings documentation for
> battery charging controller as the subnode of MAX8998 PMIC.
> It's based on current behavior of driver.
> 
> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")

Why is this here?  This patch doesn't look like a fix to me.

> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> index 23a3650ff2a2..f95610afb57f 100644
> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
>    for buck2 regulator that can be selected using dvs gpio.
>  
> +Charger: Configuration for battery charging controller should be added
> +inside a child node named 'charger'.
> +  Required properties:
> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: 0, 10 - 45
> +
> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 100, 150, 200
> +
> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 5, 6, 7
> +
>  Regulators: All the regulators of MAX8998 to be instantiated shall be
>  listed in a child node named 'regulators'. Each regulator is represented
>  by a child node of the 'regulators' node.
> @@ -99,6 +114,13 @@ Example:
>  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
>  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
>  
> +		/* Charger configuration */
> +		charger {
> +			max8998,charge-eoc = <0>;
> +			max8998,charge-restart = <(-1)>;
> +			max8998,charge-timeout = <7>;
> +		};
> +
>  		/* Regulators to instantiate */
>  		regulators {
>  			ldo2_reg: LDO2 {

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] dt-bindings: mfd: max8998: Add charger subnode binding

[Paweł Chmiel]

On Monday, April 30, 2018 1:30:36 PM CEST Lee Jones wrote:
> On Fri, 27 Apr 2018, Paweł Chmiel wrote:
> 
> > This patch adds devicetree bindings documentation for
> > battery charging controller as the subnode of MAX8998 PMIC.
> > It's based on current behavior of driver.
> > 
> > Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> 
> Why is this here?  This patch doesn't look like a fix to me.
Hi
I though that if previous patch, which is adding missing device tree parsing into charger driver,
has fixes tag, this patch (which is documenting it), should also contain that tag.
If it shouldn't be here, i can fix this in v2.

Thanks for feedback (this is my second patchset for Linux Kernel)
> 
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> > index 23a3650ff2a2..f95610afb57f 100644
> > --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> > +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> > @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
> >  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
> >    for buck2 regulator that can be selected using dvs gpio.
> >  
> > +Charger: Configuration for battery charging controller should be added
> > +inside a child node named 'charger'.
> > +  Required properties:
> > +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: 0, 10 - 45
> > +
> > +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 100, 150, 200
> > +
> > +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 5, 6, 7
> > +
> >  Regulators: All the regulators of MAX8998 to be instantiated shall be
> >  listed in a child node named 'regulators'. Each regulator is represented
> >  by a child node of the 'regulators' node.
> > @@ -99,6 +114,13 @@ Example:
> >  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
> >  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
> >  
> > +		/* Charger configuration */
> > +		charger {
> > +			max8998,charge-eoc = <0>;
> > +			max8998,charge-restart = <(-1)>;
> > +			max8998,charge-timeout = <7>;
> > +		};
> > +
> >  		/* Regulators to instantiate */
> >  		regulators {
> >  			ldo2_reg: LDO2 {
> 
> 

Re: [PATCH 4/4] dt-bindings: mfd: max8998: Add charger subnode binding

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

Hi,

On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
> This patch adds devicetree bindings documentation for
> battery charging controller as the subnode of MAX8998 PMIC.
> It's based on current behavior of driver.
> 
> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> index 23a3650ff2a2..f95610afb57f 100644
> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
>    for buck2 regulator that can be selected using dvs gpio.
>  
> +Charger: Configuration for battery charging controller should be added
> +inside a child node named 'charger'.
> +  Required properties:
> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: 0, 10 - 45
> +
> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 100, 150, 200
> +
> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> +    remain value set from bootloader or default value will be used.
> +    Valid values: -1, 0, 5, 6, 7

What are those values? seconds?

> +
>  Regulators: All the regulators of MAX8998 to be instantiated shall be
>  listed in a child node named 'regulators'. Each regulator is represented
>  by a child node of the 'regulators' node.
> @@ -99,6 +114,13 @@ Example:
>  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
>  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
>  
> +		/* Charger configuration */
> +		charger {
> +			max8998,charge-eoc = <0>;
> +			max8998,charge-restart = <(-1)>;
> +			max8998,charge-timeout = <7>;
> +		};
> +
>  		/* Regulators to instantiate */
>  		regulators {
>  			ldo2_reg: LDO2 {
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] dt-bindings: mfd: max8998: Add charger subnode binding

[Paweł Chmiel]

On Tuesday, May 1, 2018 2:45:40 PM CEST Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
> > This patch adds devicetree bindings documentation for
> > battery charging controller as the subnode of MAX8998 PMIC.
> > It's based on current behavior of driver.
> > 
> > Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> > index 23a3650ff2a2..f95610afb57f 100644
> > --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> > +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> > @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
> >  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
> >    for buck2 regulator that can be selected using dvs gpio.
> >  
> > +Charger: Configuration for battery charging controller should be added
> > +inside a child node named 'charger'.
> > +  Required properties:
> > +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: 0, 10 - 45
> > +
> > +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 100, 150, 200
> > +
> > +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> > +    remain value set from bootloader or default value will be used.
> > +    Valid values: -1, 0, 5, 6, 7
> 
> What are those values? seconds?
> 
Honestly i don't know. I've just documented values accepted currently by charger driver,
so we can use it from devicetree.
I couldn't find any max8998 datasheet with this information (units, possible values etc for those properties).

If this is not acceptable, i can drop this patch.

> > +
> >  Regulators: All the regulators of MAX8998 to be instantiated shall be
> >  listed in a child node named 'regulators'. Each regulator is represented
> >  by a child node of the 'regulators' node.
> > @@ -99,6 +114,13 @@ Example:
> >  		max8998,pmic-buck2-dvs-gpio = <&gpx0 0 3 0 0>; /* SET3 */
> >  		max8998,pmic-buck2-dvs-voltage = <1350000>, <1300000>;
> >  
> > +		/* Charger configuration */
> > +		charger {
> > +			max8998,charge-eoc = <0>;
> > +			max8998,charge-restart = <(-1)>;
> > +			max8998,charge-timeout = <7>;
> > +		};
> > +
> >  		/* Regulators to instantiate */
> >  		regulators {
> >  			ldo2_reg: LDO2 {
> 

Re: [PATCH 4/4] dt-bindings: mfd: max8998: Add charger subnode binding

[Sylwester Nawrocki]

Hi,

On 5/1/18 16:43, Paweł Chmiel wrote:
> On Tuesday, May 1, 2018 2:45:40 PM CEST Sebastian Reichel wrote:
>> On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
>>> This patch adds devicetree bindings documentation for
>>> battery charging controller as the subnode of MAX8998 PMIC.
>>> It's based on current behavior of driver.
>>>
>>> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
>>> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
>>>  1 file changed, 22 insertions(+)

>>> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
>>> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
>>> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
>>>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
>>>    for buck2 regulator that can be selected using dvs gpio.
>>>  
>>> +Charger: Configuration for battery charging controller should be added
>>> +inside a child node named 'charger'.
>>> +  Required properties:
>>> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
>>> +    remain value set from bootloader or default value will be used.
>>> +    Valid values: 0, 10 - 45
>>> +
>>> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
>>> +    remain value set from bootloader or default value will be used.
>>> +    Valid values: -1, 0, 100, 150, 200

Perhaps change the property name to max8998,charge-restart-threshold, 
in include/linux/mfd/max8998.h we have:

 * @restart: Restart Level in mV: 100, 150, 200, and -1 for disable.
 *   If it equals 0, leave it unchanged.

Then we could make it an optional property:

 - max8998,charge-restart-threshold: Charge restart threshold in millivolts. 
   Valid values are: 0, 100, 150, 200. If the value equals 0 the charger 
   restart will be disabled.

If the property is missing the charger restart threshold configuration would
be left unchanged.

>>> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
>>> +    remain value set from bootloader or default value will be used.
>>> +    Valid values: -1, 0, 5, 6, 7
>>
>> What are those values? seconds?
>>
> Honestly i don't know. I've just documented values accepted currently 
> by charger driver, so we can use it from devicetree.
> I couldn't find any max8998 datasheet with this information (units, possible 
> values etc for those properties).

The charge timeout is in hours, as described in include/linux/mfd/max8998.h:


"* @timeout: Full Timeout in hours: 5, 6, 7, and -1 for disable.
 *   If it equals 0, leave it unchanged.
 *   Otherwise, leave it unchanged."

We could change description of the property to something along the lines of:

Optional properties:

  - max8998,charge-timeout: Charge timeout in hours. Valid values are:
    0, 5, 6, 7. If the value is 0 the charge timer will be disabled.

Then if the property is missing the driver will leave charge timer configuration
unchanged.

-- 
Regards,
Sylwester

Re: [PATCH 4/4] dt-bindings: mfd: max8998: Add charger subnode binding

[Paweł Chmiel]

Hi

On Thursday, 10 January 2019 18:47:11 CET Sylwester Nawrocki wrote:
> Hi,
> 
> On 5/1/18 16:43, Paweł Chmiel wrote:
> > On Tuesday, May 1, 2018 2:45:40 PM CEST Sebastian Reichel wrote:
> >> On Fri, Apr 27, 2018 at 06:03:02PM +0200, Paweł Chmiel wrote:
> >>> This patch adds devicetree bindings documentation for
> >>> battery charging controller as the subnode of MAX8998 PMIC.
> >>> It's based on current behavior of driver.
> >>>
> >>> Fixes: ee999fb3f17f ("mfd: max8998: Add support for Device Tree")
> >>> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/mfd/max8998.txt | 22 ++++++++++++++++++++++
> >>>  1 file changed, 22 insertions(+)
> 
> >>> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> >>> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> >>> @@ -50,6 +50,21 @@ Additional properties required if max8998,pmic-buck2-dvs-gpio is defined:
> >>>  - max8998,pmic-buck2-dvs-voltage: An array of 2 voltage values in microvolts
> >>>    for buck2 regulator that can be selected using dvs gpio.
> >>>  
> >>> +Charger: Configuration for battery charging controller should be added
> >>> +inside a child node named 'charger'.
> >>> +  Required properties:
> >>> +  - max8998,charge-eoc: Setup "End of Charge". If value equals 0,
> >>> +    remain value set from bootloader or default value will be used.
> >>> +    Valid values: 0, 10 - 45
> >>> +
> >>> +  - max8998,charge-restart: Setup "Charge Restart Level". If value equals 0,
> >>> +    remain value set from bootloader or default value will be used.
> >>> +    Valid values: -1, 0, 100, 150, 200
> 
> Perhaps change the property name to max8998,charge-restart-threshold, 
> in include/linux/mfd/max8998.h we have:
> 
>  * @restart: Restart Level in mV: 100, 150, 200, and -1 for disable.
>  *   If it equals 0, leave it unchanged.
> 
> Then we could make it an optional property:
> 
>  - max8998,charge-restart-threshold: Charge restart threshold in millivolts. 
>    Valid values are: 0, 100, 150, 200. If the value equals 0 the charger 
>    restart will be disabled.
> 
> If the property is missing the charger restart threshold configuration would
> be left unchanged.
> 
Strange, i've looked at max8998 and didn't saw information that those values are in mV (maybe it was from vendor sources).

> >>> +  - max8998,charge-timeout: Setup "Charge Full Timeout". If value equals 0,
> >>> +    remain value set from bootloader or default value will be used.
> >>> +    Valid values: -1, 0, 5, 6, 7
> >>
> >> What are those values? seconds?
> >>
> > Honestly i don't know. I've just documented values accepted currently 
> > by charger driver, so we can use it from devicetree.
> > I couldn't find any max8998 datasheet with this information (units, possible 
> > values etc for those properties).
> 
> The charge timeout is in hours, as described in include/linux/mfd/max8998.h:
> 
> 
> "* @timeout: Full Timeout in hours: 5, 6, 7, and -1 for disable.
>  *   If it equals 0, leave it unchanged.
>  *   Otherwise, leave it unchanged."
> 
> We could change description of the property to something along the lines of:
> 
> Optional properties:
> 
>   - max8998,charge-timeout: Charge timeout in hours. Valid values are:
>     0, 5, 6, 7. If the value is 0 the charge timer will be disabled.
> 
> Then if the property is missing the driver will leave charge timer configuration
> unchanged.
> 
> 
Thanks Sylwester Nawrocki for all those hints (mostly regarding units of those magic numbers/values). I'll prepare new version of those patches with all hints included.