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
From: Tomasz Figa <[email protected]>
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 <[email protected]>
Signed-off-by: Paweł Chmiel <[email protected]>
---
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
This patch adds missing code for reading charger configuration
from devicetree.
Signed-off-by: Paweł Chmiel <[email protected]>
---
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
This patch adds devicetree bindings documentation for
battery charging controller as the subnode of MAX8998 PMIC.
Signed-off-by: Paweł Chmiel <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > ---
> > 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 {
>
Hi,
On Tue, Jul 17, 2018 at 06:05:07PM +0200, Paweł Chmiel wrote:
> From: Tomasz Figa <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Paweł Chmiel <[email protected]>
> ---
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
>
On Thu, 13 Sep 2018 at 16:56, Paweł Chmiel
<[email protected]> 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 <[email protected]>
> > > ---
> > > 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