[PATCH v3 0/3] power: supply: max8998-charger: Device Tree support

[PATCH v3 0/3] power: supply: max8998-charger: Device Tree support

[Paweł Chmiel]

This patch series compose of 3 patches.

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

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

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

All patches has been tested on, Samsung Galaxy S (i9000) phone.

Changes from v2:
  - Make charge-restart-level-microvolt and charge-timeout-hours
    properties optional. If they're not present, assume they're disabled.

Changes from v1:
  - Removed unneeded Fixes tag
  - Correct description of all charger values
  - Added missing property unit for charger properties
  - Removed already applied patch

Paweł Chmiel (2):
  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 | 25 +++++++++
 drivers/power/supply/max8998_charger.c            | 62 ++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v3 1/3] 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 v3 2/3] power: supply: max8998-charger: Parse device tree for required data.

[Paweł Chmiel]

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

Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
Changes from v2:
  - Make restart level and charge timeout properties optional.
    If they're not present in devicetree, assume they're disabled.

Changes from v1:
  - Removed unneeded Fixes tag
  - Use new property names
---
 drivers/power/supply/max8998_charger.c | 60 ++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/power/supply/max8998_charger.c b/drivers/power/supply/max8998_charger.c
index 66438029bdd0..2faba2b99639 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,59 @@ 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-percent",
+					&pdata->eoc);
+	if (ret < 0) {
+		dev_err(iodev->dev,
+			"Could not find max8998,charge-eoc-percent in devicetree\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(charger_np,
+					"max8998,charge-restart-level-microvolt",
+					&pdata->restart);
+	if (ret < 0) {
+		if (ret != -EINVAL) {
+			dev_err(iodev->dev,
+				"Failed to read max8998,charge-restart-level-microvolt\n");
+			return ret;
+		}
+
+		pdata->restart = -1;
+		dev_dbg(iodev->dev, "Charge Restart Level disabled\n");
+	}
+
+	ret = of_property_read_u32(charger_np,
+					"max8998,charge-timeout-hours",
+					&pdata->timeout);
+	if (ret < 0) {
+		if (ret != -EINVAL) {
+			dev_err(iodev->dev,
+				"Failed to read max8998,charge-timeout-hours\n");
+			return ret;
+		}
+
+		pdata->timeout = -1;
+		dev_dbg(iodev->dev, "Charge Full Timeout disabled\n");
+	}
+
+	return 0;
+}
+
 static int max8998_battery_probe(struct platform_device *pdev)
 {
 	struct max8998_dev *iodev = dev_get_drvdata(pdev->dev.parent);
@@ -96,6 +150,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 v3 3/3] 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.

Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
Changes from v2:
  - Make charge-restart-level-microvolt optional.
  - Make charge-timeout-hours optional.

Changes from v1:
  - Removed unneeded Fixes tag
  - Correct description of all charger values
  - Added missing property unit
---
 Documentation/devicetree/bindings/mfd/max8998.txt | 25 +++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
index 23a3650ff2a2..264040f4ad15 100644
--- a/Documentation/devicetree/bindings/mfd/max8998.txt
+++ b/Documentation/devicetree/bindings/mfd/max8998.txt
@@ -50,6 +50,24 @@ 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-percent: Setup End of Charge Level.
+    If value equals 0, leave it unchanged.
+    Otherwise it should be value from 10 to 45 by 5 step.
+
+  Optional properties:
+  - max8998,charge-restart-level-microvolt: Setup Charge Restart Level.
+    If property is not present, this will be disabled.
+    If value equals 0, leave it unchanged.
+    Otherwise it should be one of following values: 100, 150, 200.
+
+  - max8998,charge-timeout-hours: Setup Charge Full Timeout.
+    If property is not present, this will be disabled.
+    If value equals 0, leave it unchanged.
+    Otherwise it should be one of following values: 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 +117,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-percent = <0>;
+			max8998,charge-restart-level-microvolt = <100>;
+			max8998,charge-timeout-hours = <7>;
+		};
+
 		/* Regulators to instantiate */
 		regulators {
 			ldo2_reg: LDO2 {
-- 
2.7.4

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

[Rob Herring]

On Tue, Jul 17, 2018 at 06:05:09PM +0200, Paweł Chmiel wrote:
> This patch adds devicetree bindings documentation for
> battery charging controller as the subnode of MAX8998 PMIC.
> 
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
> Changes from v2:
>   - Make charge-restart-level-microvolt optional.
>   - Make charge-timeout-hours optional.
> 
> Changes from v1:
>   - Removed unneeded Fixes tag
>   - Correct description of all charger values
>   - Added missing property unit
> ---
>  Documentation/devicetree/bindings/mfd/max8998.txt | 25 +++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> index 23a3650ff2a2..264040f4ad15 100644
> --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> @@ -50,6 +50,24 @@ 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-percent: Setup End of Charge Level.

This should have a vendor prefix and max8998 is not a vendor. Don't 
continue that pattern even if we already have some properties like that.

These seem like perhaps they should be common charger properties.

> +    If value equals 0, leave it unchanged.
> +    Otherwise it should be value from 10 to 45 by 5 step.
> +
> +  Optional properties:
> +  - max8998,charge-restart-level-microvolt: Setup Charge Restart Level.
> +    If property is not present, this will be disabled.
> +    If value equals 0, leave it unchanged.
> +    Otherwise it should be one of following values: 100, 150, 200.
> +
> +  - max8998,charge-timeout-hours: Setup Charge Full Timeout.
> +    If property is not present, this will be disabled.
> +    If value equals 0, leave it unchanged.
> +    Otherwise it should be one of following values: 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 +117,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-percent = <0>;
> +			max8998,charge-restart-level-microvolt = <100>;
> +			max8998,charge-timeout-hours = <7>;
> +		};
> +
>  		/* Regulators to instantiate */
>  		regulators {
>  			ldo2_reg: LDO2 {
> -- 
> 2.7.4
> 

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

[Paweł Chmiel]

On Tuesday, August 7, 2018 11:57:42 AM CEST Rob Herring wrote:
> On Tue, Jul 17, 2018 at 06:05:09PM +0200, Paweł Chmiel wrote:
> > This patch adds devicetree bindings documentation for
> > battery charging controller as the subnode of MAX8998 PMIC.
> > 
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> > Changes from v2:
> >   - Make charge-restart-level-microvolt optional.
> >   - Make charge-timeout-hours optional.
> > 
> > Changes from v1:
> >   - Removed unneeded Fixes tag
> >   - Correct description of all charger values
> >   - Added missing property unit
> > ---
> >  Documentation/devicetree/bindings/mfd/max8998.txt | 25 +++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> > index 23a3650ff2a2..264040f4ad15 100644
> > --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> > +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> > @@ -50,6 +50,24 @@ 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-percent: Setup End of Charge Level.
> 
> This should have a vendor prefix and max8998 is not a vendor. Don't 
> continue that pattern even if we already have some properties like that.
> 
> These seem like perhaps they should be common charger properties.
Where i could find such properties (or any other driver which is using those as an example) ?
Looking at few existing drivers, most of them have custom properties (that's why i've followed the same pattern for max8998).

Thanks
> 
> > +    If value equals 0, leave it unchanged.
> > +    Otherwise it should be value from 10 to 45 by 5 step.
> > +
> > +  Optional properties:
> > +  - max8998,charge-restart-level-microvolt: Setup Charge Restart Level.
> > +    If property is not present, this will be disabled.
> > +    If value equals 0, leave it unchanged.
> > +    Otherwise it should be one of following values: 100, 150, 200.
> > +
> > +  - max8998,charge-timeout-hours: Setup Charge Full Timeout.
> > +    If property is not present, this will be disabled.
> > +    If value equals 0, leave it unchanged.
> > +    Otherwise it should be one of following values: 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 +117,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-percent = <0>;
> > +			max8998,charge-restart-level-microvolt = <100>;
> > +			max8998,charge-timeout-hours = <7>;
> > +		};
> > +
> >  		/* Regulators to instantiate */
> >  		regulators {
> >  			ldo2_reg: LDO2 {
> 

Re: [PATCH v3 1/3] power: supply: max8998-charger: Fix platform data retrieval

[Sebastian Reichel]

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

Hi,

On Tue, Jul 17, 2018 at 06:05:07PM +0200, Paweł Chmiel wrote:
> 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>
> ---

Thanks, queued.

-- Sebastian

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

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

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

[Krzysztof Kozlowski]

On Thu, 13 Sep 2018 at 16:56, Paweł Chmiel
<pawel.mikolaj.chmiel@gmail.com> wrote:
>
> On Tuesday, August 7, 2018 11:57:42 AM CEST Rob Herring wrote:
> > On Tue, Jul 17, 2018 at 06:05:09PM +0200, Paweł Chmiel wrote:
> > > This patch adds devicetree bindings documentation for
> > > battery charging controller as the subnode of MAX8998 PMIC.
> > >
> > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > ---
> > > Changes from v2:
> > >   - Make charge-restart-level-microvolt optional.
> > >   - Make charge-timeout-hours optional.
> > >
> > > Changes from v1:
> > >   - Removed unneeded Fixes tag
> > >   - Correct description of all charger values
> > >   - Added missing property unit
> > > ---
> > >  Documentation/devicetree/bindings/mfd/max8998.txt | 25 +++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/max8998.txt b/Documentation/devicetree/bindings/mfd/max8998.txt
> > > index 23a3650ff2a2..264040f4ad15 100644
> > > --- a/Documentation/devicetree/bindings/mfd/max8998.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/max8998.txt
> > > @@ -50,6 +50,24 @@ 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-percent: Setup End of Charge Level.
> >
> > This should have a vendor prefix and max8998 is not a vendor. Don't
> > continue that pattern even if we already have some properties like that.
> >
> > These seem like perhaps they should be common charger properties.
> Where i could find such properties (or any other driver which is using those as an example) ?
> Looking at few existing drivers, most of them have custom properties (that's why i've followed the same pattern for max8998).

The common properties are now here:
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/power/supply/battery.txt

The property "max8998,charge-eoc-percent" seems unusual - what does
"percent" mean in such case? Usually EOC should happen at 100% of
charge so what is the point to set it as 45%?

Have in mind that current driver is platform data only so you do not
have to maintain any backwards compatibility. If the driver now has
weird meaning of "eoc" in pdata, then you can change it. Also you do
not have to map the bindings one-to-one to existing platform data. If
needed you can translate them from something meaningful in DT to
values expected by driver.

Best regards,
Krzysztof