[PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Enric Balletbo i Serra]

Add charging voltage and current characteristics to the battery DT for
proper handling of the battery by fuel-gauge and charger chips.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v2:
 - Requested by Sebastian Reichel
   - Move to its own patch and apply to simple-battery framework.
Changes since v1:
 - Requested by Rob Herring
   - Rename ti,charge-* to charge-* to be standard properties.
   - Use unit suffixes as per bindings/property-units.txt

 Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
index 63a7028..c87a439 100644
--- a/Documentation/devicetree/bindings/power/supply/battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -12,6 +12,8 @@ Optional Properties:
  - voltage-min-design-microvolt: drained battery voltage
  - energy-full-design-microwatt-hours: battery design energy
  - charge-full-design-microamp-hours: battery design capacity
+ - charge-voltage-microvolt: battery charging voltage
+ - charge-current-microamp: battery charging current
 
 Batteries must be referenced by chargers and/or fuel-gauges
 using a phandle. The phandle's property should be named
@@ -24,6 +26,8 @@ Example:
 		voltage-min-design-microvolt = <3200000>;
 		energy-full-design-microwatt-hours = <5290000>;
 		charge-full-design-microamp-hours = <1430000>;
+		charge-voltage-microvolt = <4100000>;
+		charge-current-microamp = <300000>;
 	};
 
 	charger: charger@11 {
-- 
2.9.3

[PATCH v3 2/4] power: supply: core: add charging voltage/current battery info

[Enric Balletbo i Serra]

Add the parameters to define the battery charging voltage and charging
current. Charger driver can get this information from the struct
power_supply_battery_info and apply the desired value.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v2:
 - Requested by Sebastian Reichel
   - Move to its own patch and apply to simple-battery framework.
Changes since v1:
 - Requested by Rob Herring
   - Rename ti,charge-* to charge-* to be standard properties.
   - Use unit suffixes as per bindings/property-units.txt

 drivers/power/supply/power_supply_core.c | 6 ++++++
 include/linux/power_supply.h             | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 862fa8fc..a6857c2 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -530,6 +530,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	info->energy_full_design_uwh = -EINVAL;
 	info->charge_full_design_uah = -EINVAL;
 	info->voltage_min_design_uv  = -EINVAL;
+	info->charge_voltage_uv = -EINVAL;
+	info->charge_current_ua = -EINVAL;
 
 	if (!psy->of_node) {
 		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
@@ -559,6 +561,10 @@ int power_supply_get_battery_info(struct power_supply *psy,
 			     &info->charge_full_design_uah);
 	of_property_read_u32(battery_np, "voltage-min-design-microvolt",
 			     &info->voltage_min_design_uv);
+	of_property_read_u32(battery_np, "charge-voltage-microvolt",
+			     &info->charge_voltage_uv);
+	of_property_read_u32(battery_np, "charge-current-microamp",
+			     &info->charge_current_ua);
 
 	return 0;
 }
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 8220f7b..3eea323 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -302,6 +302,8 @@ struct power_supply_battery_info {
 	int energy_full_design_uwh;	/* microWatt-hours */
 	int charge_full_design_uah;	/* microAmp-hours */
 	int voltage_min_design_uv;	/* microVolts */
+	int charge_voltage_uv;		/* microVolts */
+	int charge_current_ua;		/* microAmp */
 };
 
 extern struct atomic_notifier_head power_supply_notifier;
-- 
2.9.3

[PATCH v3 3/4] dt-bindings: power: tps65217_charger: add NTC type for battery temperature measurement.

[Enric Balletbo i Serra]

The TS pin of the TPS56217 connects to the NTC resistor in the battery
pack. By default the device is setup to support a 10-kohm but can also
be configured to support a 100-kohm. Add a propietry to configure the
connected NTC resistor. Therefore, the charger would get the wrong
temperature information.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v2:
 - Requested by Sebastian Reichel
   - Get rid of common properties and only maintain ntc-type.
Changes since v1:
 - None

 .../devicetree/bindings/power/supply/tps65217_charger.txt          | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
index a11072c..851f5c7 100644
--- a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
+++ b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
@@ -6,6 +6,12 @@ Required Properties:
              Should be <0> for the USB charger and <1> for the AC adapter.
 -interrupt-names: Should be "USB" and "AC"
 
+Optional properties:
+-ti,ntc-type: set the NTC type for battery temperature measurement. The value
+	must be 0 or 1, where:
+	  0 – 100k (curve 1, B = 3960)
+	  1 – 10k  (curve 2, B = 3480) (default)
+
 This node is a subnode of the tps65217 PMIC.
 
 Example:
@@ -14,4 +20,5 @@ Example:
 		compatible = "ti,tps65217-charger";
 		interrupts = <0>, <1>;
 		interrupt-names = "USB", "AC";
+		ti,ntc-type = <1>;
 	};
-- 
2.9.3

[PATCH v3 4/4] power: tps65217_charger: add support for NTC type, voltage and current charge

[Enric Balletbo i Serra]

Allow the possibility to configure the charge and the current voltage of
the charger and also the NTC type for battery temperature measurement.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v2:
 - Requested by Sebastian Reichel
   - Use the simple-battery framework
   - Use device_property_read_u32 instead of of_property_read_u32
Changes since v1:
 - None

 drivers/power/supply/tps65217_charger.c | 194 ++++++++++++++++++++++++++++++--
 include/linux/mfd/tps65217.h            |   2 +
 2 files changed, 188 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 1f52340..5939e77 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -39,6 +39,12 @@
 #define NUM_CHARGER_IRQS	2
 #define POLL_INTERVAL		(HZ * 2)
 
+struct tps65217_charger_platform_data {
+	int	charge_voltage_uv;
+	int	charge_current_ua;
+	int	ntc_type;
+};
+
 struct tps65217_charger {
 	struct tps65217 *tps;
 	struct device *dev;
@@ -48,16 +54,84 @@ struct tps65217_charger {
 	int	prev_online;
 
 	struct task_struct	*poll_task;
+	struct tps65217_charger_platform_data *pdata;
 };
 
 static enum power_supply_property tps65217_charger_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 };
 
-static int tps65217_config_charger(struct tps65217_charger *charger)
+static int tps65217_set_charge_current(struct tps65217_charger *charger,
+				       unsigned int uamp)
+{
+	int ret, val;
+
+	dev_dbg(charger->dev, "setting charge current to %d uA\n", uamp);
+
+	if (uamp == 300000)
+		val = 0x00;
+	else if (uamp == 400000)
+		val = 0x01;
+	else if (uamp == 500000)
+		val = 0x02;
+	else if (uamp == 700000)
+		val = 0x03;
+	else
+		return -EINVAL;
+
+	ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG3,
+				TPS65217_CHGCONFIG3_ICHRG_MASK,
+				val << TPS65217_CHGCONFIG3_ICHRG_SHIFT,
+				TPS65217_PROTECT_NONE);
+	if (ret) {
+		dev_err(charger->dev,
+			"failed to set ICHRG setting to 0x%02x (err: %d)\n",
+			val, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tps65217_set_charge_voltage(struct tps65217_charger *charger,
+				       unsigned int uvolt)
+{
+	int ret, val;
+
+	dev_dbg(charger->dev, "setting charge voltage to %d uV\n", uvolt);
+
+	if (uvolt != 4100000 && uvolt != 4150000 &&
+	    uvolt != 4200000 && uvolt != 4250000)
+		return -EINVAL;
+
+	val = (uvolt - 4100000) / 50000;
+
+	ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG2,
+				TPS65217_CHGCONFIG2_VOREG_MASK,
+				val << TPS65217_CHGCONFIG2_VOREG_SHIFT,
+				TPS65217_PROTECT_NONE);
+	if (ret) {
+		dev_err(charger->dev,
+			"failed to set VOCHG setting to 0x%02x (err: %d)\n",
+			val, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tps65217_set_ntc_type(struct tps65217_charger *charger,
+				 unsigned int ntc)
 {
 	int ret;
 
+	dev_dbg(charger->dev, "setting NTC type to %d\n", ntc);
+
+	if (ntc != 0 && ntc != 1)
+		return -EINVAL;
+
 	/*
 	 * tps65217 rev. G, p. 31 (see p. 32 for NTC schematic)
 	 *
@@ -74,14 +148,57 @@ static int tps65217_config_charger(struct tps65217_charger *charger)
 	 * NTC TYPE (for battery temperature measurement)
 	 *   0 – 100k (curve 1, B = 3960)
 	 *   1 – 10k  (curve 2, B = 3480) (default on reset)
-	 *
 	 */
-	ret = tps65217_clear_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
-				  TPS65217_CHGCONFIG1_NTC_TYPE,
-				  TPS65217_PROTECT_NONE);
+	if (ntc) {
+		ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
+					TPS65217_CHGCONFIG1_NTC_TYPE,
+					TPS65217_CHGCONFIG1_NTC_TYPE,
+					TPS65217_PROTECT_NONE);
+		if (ret) {
+			dev_err(charger->dev,
+				"failed to set NTC type to 10K: %d\n", ret);
+			return ret;
+		}
+	} else {
+		ret = tps65217_clear_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
+					  TPS65217_CHGCONFIG1_NTC_TYPE,
+					  TPS65217_PROTECT_NONE);
+		if (ret) {
+			dev_err(charger->dev,
+				"failed to set NTC type to 100K: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int tps65217_config_charger(struct tps65217_charger *charger)
+{
+	int ret;
+	struct tps65217_charger_platform_data *pdata = charger->pdata;
+
+	if (!charger->pdata)
+		return -EINVAL;
+
+	ret = tps65217_set_charge_voltage(charger, pdata->charge_voltage_uv);
+	if (ret) {
+		dev_err(charger->dev,
+			"failed to set charge voltage setting: %d\n", ret);
+		return ret;
+	}
+
+	ret = tps65217_set_charge_current(charger, pdata->charge_current_ua);
+	if (ret) {
+		dev_err(charger->dev,
+			"failed to set charge current setting: %d\n", ret);
+		return ret;
+	}
+
+	ret = tps65217_set_ntc_type(charger, pdata->ntc_type);
 	if (ret) {
 		dev_err(charger->dev,
-			"failed to set 100k NTC setting: %d\n", ret);
+			"failed to set NTC type setting: %d\n", ret);
 		return ret;
 	}
 
@@ -118,11 +235,23 @@ static int tps65217_charger_get_property(struct power_supply *psy,
 					 union power_supply_propval *val)
 {
 	struct tps65217_charger *charger = power_supply_get_drvdata(psy);
+	struct tps65217_charger_platform_data *pdata = charger->pdata;
 
 	if (psp == POWER_SUPPLY_PROP_ONLINE) {
 		val->intval = charger->online;
 		return 0;
 	}
+
+	if (psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE) {
+		val->intval = pdata->charge_voltage_uv;
+		return 0;
+	}
+
+	if (psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT) {
+		val->intval = pdata->charge_current_ua;
+		return 0;
+	}
+
 	return -EINVAL;
 }
 
@@ -185,6 +314,49 @@ static int tps65217_charger_poll_task(void *data)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static struct tps65217_charger_platform_data *tps65217_charger_pdata_init(
+		struct tps65217_charger *charger)
+{
+	struct tps65217_charger_platform_data *pdata;
+	struct power_supply_battery_info info = {};
+	int ret;
+
+	pdata = devm_kzalloc(charger->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	/*
+	 * If battery info is not supplied just ignore and program default
+	 * values.
+	 */
+	power_supply_get_battery_info(charger->psy, &info);
+
+	if (info.charge_voltage_uv > 0)
+		pdata->charge_voltage_uv = info.charge_voltage_uv;
+	else
+		pdata->charge_voltage_uv = 4100000;
+
+	if (info.charge_current_ua > 0)
+		pdata->charge_current_ua = info.charge_current_ua;
+	else
+		pdata->charge_current_ua = 500000;
+
+	ret = device_property_read_u32(charger->dev, "ti,ntc-type",
+				       &pdata->ntc_type);
+	if (ret)
+		pdata->ntc_type = 1;	/* 10k  (curve 2, B = 3480) */
+
+	return pdata;
+}
+#else /* CONFIG_OF */
+static struct tps65217_charger_platform_data *tps65217_charger_pdata_init(
+		struct tps65217_charger *charger)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
 static const struct power_supply_desc tps65217_charger_desc = {
 	.name			= "tps65217-charger",
 	.type			= POWER_SUPPLY_TYPE_MAINS,
@@ -222,8 +394,11 @@ static int tps65217_charger_probe(struct platform_device *pdev)
 		return PTR_ERR(charger->psy);
 	}
 
-	irq[0] = platform_get_irq_byname(pdev, "USB");
-	irq[1] = platform_get_irq_byname(pdev, "AC");
+	charger->pdata = tps65217_charger_pdata_init(charger);
+	if (IS_ERR(charger->pdata)) {
+		dev_err(charger->dev, "failed: getting platform data\n");
+		return PTR_ERR(charger->pdata);
+	}
 
 	ret = tps65217_config_charger(charger);
 	if (ret < 0) {
@@ -231,6 +406,9 @@ static int tps65217_charger_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	irq[0] = platform_get_irq_byname(pdev, "USB");
+	irq[1] = platform_get_irq_byname(pdev, "AC");
+
 	/* Create a polling thread if an interrupt is invalid */
 	if (irq[0] < 0 || irq[1] < 0) {
 		poll_task = kthread_run(tps65217_charger_poll_task,
diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
index eac2857..d040062 100644
--- a/include/linux/mfd/tps65217.h
+++ b/include/linux/mfd/tps65217.h
@@ -103,8 +103,10 @@
 #define TPS65217_CHGCONFIG2_DYNTMR	BIT(7)
 #define TPS65217_CHGCONFIG2_VPREGHG	BIT(6)
 #define TPS65217_CHGCONFIG2_VOREG_MASK	0x30
+#define TPS65217_CHGCONFIG2_VOREG_SHIFT	4
 
 #define TPS65217_CHGCONFIG3_ICHRG_MASK	0xC0
+#define TPS65217_CHGCONFIG3_ICHRG_SHIFT	6
 #define TPS65217_CHGCONFIG3_DPPMTH_MASK	0x30
 #define TPS65217_CHGCONFIG2_PCHRGT	BIT(3)
 #define TPS65217_CHGCONFIG2_TERMIF	0x06
-- 
2.9.3

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,

On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Add charging voltage and current characteristics to the battery DT for
> proper handling of the battery by fuel-gauge and charger chips.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>    - Move to its own patch and apply to simple-battery framework.
> Changes since v1:
>  - Requested by Rob Herring
>    - Rename ti,charge-* to charge-* to be standard properties.
>    - Use unit suffixes as per bindings/property-units.txt
>
>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
> index 63a7028..c87a439 100644
> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -12,6 +12,8 @@ Optional Properties:
>   - voltage-min-design-microvolt: drained battery voltage
>   - energy-full-design-microwatt-hours: battery design energy
>   - charge-full-design-microamp-hours: battery design capacity
> + - charge-voltage-microvolt: battery charging voltage
> + - charge-current-microamp: battery charging current

I think you mean constant-charge-* which is how you surface these
properties in your tps65217_charger patch.

I'll add these to v14 of my patchset which adds simple-battery
support. Rob requested a single patch for this file.

I've been waiting for feedback on v13.2 from Sebastian. If I don't
hear from him within a few days, I'll post v14.

>  Batteries must be referenced by chargers and/or fuel-gauges
>  using a phandle. The phandle's property should be named
> @@ -24,6 +26,8 @@ Example:
>                 voltage-min-design-microvolt = <3200000>;
>                 energy-full-design-microwatt-hours = <5290000>;
>                 charge-full-design-microamp-hours = <1430000>;
> +               charge-voltage-microvolt = <4100000>;
> +               charge-current-microamp = <300000>;
>         };
>
>         charger: charger@11 {
> --
> 2.9.3
>

Re: [PATCH v3 2/4] power: supply: core: add charging voltage/current battery info

[Liam Breck]

Hi Enric,

I'll also incorporate these changes into v14 of my patchset. (See my prev msg.)


On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Add the parameters to define the battery charging voltage and charging
> current. Charger driver can get this information from the struct
> power_supply_battery_info and apply the desired value.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>    - Move to its own patch and apply to simple-battery framework.
> Changes since v1:
>  - Requested by Rob Herring
>    - Rename ti,charge-* to charge-* to be standard properties.
>    - Use unit suffixes as per bindings/property-units.txt
>
>  drivers/power/supply/power_supply_core.c | 6 ++++++
>  include/linux/power_supply.h             | 2 ++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 862fa8fc..a6857c2 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -530,6 +530,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
>         info->energy_full_design_uwh = -EINVAL;
>         info->charge_full_design_uah = -EINVAL;
>         info->voltage_min_design_uv  = -EINVAL;
> +       info->charge_voltage_uv = -EINVAL;
> +       info->charge_current_ua = -EINVAL;
>
>         if (!psy->of_node) {
>                 dev_warn(&psy->dev, "%s currently only supports devicetree\n",
> @@ -559,6 +561,10 @@ int power_supply_get_battery_info(struct power_supply *psy,
>                              &info->charge_full_design_uah);
>         of_property_read_u32(battery_np, "voltage-min-design-microvolt",
>                              &info->voltage_min_design_uv);
> +       of_property_read_u32(battery_np, "charge-voltage-microvolt",
> +                            &info->charge_voltage_uv);
> +       of_property_read_u32(battery_np, "charge-current-microamp",
> +                            &info->charge_current_ua);
>
>         return 0;
>  }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 8220f7b..3eea323 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -302,6 +302,8 @@ struct power_supply_battery_info {
>         int energy_full_design_uwh;     /* microWatt-hours */
>         int charge_full_design_uah;     /* microAmp-hours */
>         int voltage_min_design_uv;      /* microVolts */
> +       int charge_voltage_uv;          /* microVolts */
> +       int charge_current_ua;          /* microAmp */
>  };
>
>  extern struct atomic_notifier_head power_supply_notifier;
> --
> 2.9.3
>

Re: [PATCH v3 3/4] dt-bindings: power: tps65217_charger: add NTC type for battery temperature measurement.

[Liam Breck]

Hi Enric,

On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> The TS pin of the TPS56217 connects to the NTC resistor in the battery
> pack. By default the device is setup to support a 10-kohm but can also
> be configured to support a 100-kohm. Add a propietry to configure the
> connected NTC resistor. Therefore, the charger would get the wrong
> temperature information.

This also needs a monitored-battery property, and a battery node in
the example. See https://patchwork.kernel.org/patch/9710803/


> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>    - Get rid of common properties and only maintain ntc-type.
> Changes since v1:
>  - None
>
>  .../devicetree/bindings/power/supply/tps65217_charger.txt          | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> index a11072c..851f5c7 100644
> --- a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> +++ b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> @@ -6,6 +6,12 @@ Required Properties:
>               Should be <0> for the USB charger and <1> for the AC adapter.
>  -interrupt-names: Should be "USB" and "AC"
>
> +Optional properties:
> +-ti,ntc-type: set the NTC type for battery temperature measurement. The value
> +       must be 0 or 1, where:
> +         0 – 100k (curve 1, B = 3960)
> +         1 – 10k  (curve 2, B = 3480) (default)
> +
>  This node is a subnode of the tps65217 PMIC.
>
>  Example:
> @@ -14,4 +20,5 @@ Example:
>                 compatible = "ti,tps65217-charger";
>                 interrupts = <0>, <1>;
>                 interrupt-names = "USB", "AC";
> +               ti,ntc-type = <1>;
>         };
> --
> 2.9.3
>

Re: [PATCH v3 4/4] power: tps65217_charger: add support for NTC type, voltage and current charge

[Liam Breck]

Hi Enric,


On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Allow the possibility to configure the charge and the current voltage of
> the charger and also the NTC type for battery temperature measurement.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>    - Use the simple-battery framework
>    - Use device_property_read_u32 instead of of_property_read_u32
> Changes since v1:
>  - None
>
>  drivers/power/supply/tps65217_charger.c | 194 ++++++++++++++++++++++++++++++--
>  include/linux/mfd/tps65217.h            |   2 +
>  2 files changed, 188 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
> index 1f52340..5939e77 100644
> --- a/drivers/power/supply/tps65217_charger.c
> +++ b/drivers/power/supply/tps65217_charger.c
> @@ -39,6 +39,12 @@
>  #define NUM_CHARGER_IRQS       2
>  #define POLL_INTERVAL          (HZ * 2)
>
> +struct tps65217_charger_platform_data {
> +       int     charge_voltage_uv;
> +       int     charge_current_ua;
> +       int     ntc_type;
> +};
> +

It's not my department, but maybe this should be called devprops or
similar, since platform_data refers to the pre-devicetree config
scheme?


>  struct tps65217_charger {
>         struct tps65217 *tps;
>         struct device *dev;
> @@ -48,16 +54,84 @@ struct tps65217_charger {
>         int     prev_online;
>
>         struct task_struct      *poll_task;
> +       struct tps65217_charger_platform_data *pdata;
>  };
>
>  static enum power_supply_property tps65217_charger_props[] = {
>         POWER_SUPPLY_PROP_ONLINE,
> +       POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> +       POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
>  };
>
> -static int tps65217_config_charger(struct tps65217_charger *charger)
> +static int tps65217_set_charge_current(struct tps65217_charger *charger,
> +                                      unsigned int uamp)
> +{
> +       int ret, val;
> +
> +       dev_dbg(charger->dev, "setting charge current to %d uA\n", uamp);
> +
> +       if (uamp == 300000)
> +               val = 0x00;
> +       else if (uamp == 400000)
> +               val = 0x01;
> +       else if (uamp == 500000)
> +               val = 0x02;
> +       else if (uamp == 700000)
> +               val = 0x03;
> +       else
> +               return -EINVAL;
> +
> +       ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG3,
> +                               TPS65217_CHGCONFIG3_ICHRG_MASK,
> +                               val << TPS65217_CHGCONFIG3_ICHRG_SHIFT,
> +                               TPS65217_PROTECT_NONE);
> +       if (ret) {
> +               dev_err(charger->dev,
> +                       "failed to set ICHRG setting to 0x%02x (err: %d)\n",
> +                       val, ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int tps65217_set_charge_voltage(struct tps65217_charger *charger,
> +                                      unsigned int uvolt)
> +{
> +       int ret, val;
> +
> +       dev_dbg(charger->dev, "setting charge voltage to %d uV\n", uvolt);
> +
> +       if (uvolt != 4100000 && uvolt != 4150000 &&
> +           uvolt != 4200000 && uvolt != 4250000)
> +               return -EINVAL;
> +
> +       val = (uvolt - 4100000) / 50000;
> +
> +       ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG2,
> +                               TPS65217_CHGCONFIG2_VOREG_MASK,
> +                               val << TPS65217_CHGCONFIG2_VOREG_SHIFT,
> +                               TPS65217_PROTECT_NONE);
> +       if (ret) {
> +               dev_err(charger->dev,
> +                       "failed to set VOCHG setting to 0x%02x (err: %d)\n",
> +                       val, ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int tps65217_set_ntc_type(struct tps65217_charger *charger,
> +                                unsigned int ntc)
>  {
>         int ret;
>
> +       dev_dbg(charger->dev, "setting NTC type to %d\n", ntc);
> +
> +       if (ntc != 0 && ntc != 1)
> +               return -EINVAL;
> +
>         /*
>          * tps65217 rev. G, p. 31 (see p. 32 for NTC schematic)
>          *
> @@ -74,14 +148,57 @@ static int tps65217_config_charger(struct tps65217_charger *charger)
>          * NTC TYPE (for battery temperature measurement)
>          *   0 – 100k (curve 1, B = 3960)
>          *   1 – 10k  (curve 2, B = 3480) (default on reset)
> -        *
>          */
> -       ret = tps65217_clear_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
> -                                 TPS65217_CHGCONFIG1_NTC_TYPE,
> -                                 TPS65217_PROTECT_NONE);
> +       if (ntc) {
> +               ret = tps65217_set_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
> +                                       TPS65217_CHGCONFIG1_NTC_TYPE,
> +                                       TPS65217_CHGCONFIG1_NTC_TYPE,
> +                                       TPS65217_PROTECT_NONE);
> +               if (ret) {
> +                       dev_err(charger->dev,
> +                               "failed to set NTC type to 10K: %d\n", ret);
> +                       return ret;
> +               }
> +       } else {
> +               ret = tps65217_clear_bits(charger->tps, TPS65217_REG_CHGCONFIG1,
> +                                         TPS65217_CHGCONFIG1_NTC_TYPE,
> +                                         TPS65217_PROTECT_NONE);
> +               if (ret) {
> +                       dev_err(charger->dev,
> +                               "failed to set NTC type to 100K: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int tps65217_config_charger(struct tps65217_charger *charger)
> +{
> +       int ret;
> +       struct tps65217_charger_platform_data *pdata = charger->pdata;
> +
> +       if (!charger->pdata)
> +               return -EINVAL;
> +
> +       ret = tps65217_set_charge_voltage(charger, pdata->charge_voltage_uv);
> +       if (ret) {
> +               dev_err(charger->dev,
> +                       "failed to set charge voltage setting: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = tps65217_set_charge_current(charger, pdata->charge_current_ua);
> +       if (ret) {
> +               dev_err(charger->dev,
> +                       "failed to set charge current setting: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = tps65217_set_ntc_type(charger, pdata->ntc_type);
>         if (ret) {
>                 dev_err(charger->dev,
> -                       "failed to set 100k NTC setting: %d\n", ret);
> +                       "failed to set NTC type setting: %d\n", ret);
>                 return ret;
>         }
>
> @@ -118,11 +235,23 @@ static int tps65217_charger_get_property(struct power_supply *psy,
>                                          union power_supply_propval *val)
>  {
>         struct tps65217_charger *charger = power_supply_get_drvdata(psy);
> +       struct tps65217_charger_platform_data *pdata = charger->pdata;
>
>         if (psp == POWER_SUPPLY_PROP_ONLINE) {
>                 val->intval = charger->online;
>                 return 0;
>         }
> +
> +       if (psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE) {
> +               val->intval = pdata->charge_voltage_uv;
> +               return 0;
> +       }
> +
> +       if (psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT) {
> +               val->intval = pdata->charge_current_ua;
> +               return 0;
> +       }
> +
>         return -EINVAL;
>  }
>
> @@ -185,6 +314,49 @@ static int tps65217_charger_poll_task(void *data)
>         return 0;
>  }
>
> +#ifdef CONFIG_OF
> +static struct tps65217_charger_platform_data *tps65217_charger_pdata_init(
> +               struct tps65217_charger *charger)
> +{
> +       struct tps65217_charger_platform_data *pdata;
> +       struct power_supply_battery_info info = {};
> +       int ret;
> +
> +       pdata = devm_kzalloc(charger->dev, sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               return ERR_PTR(-ENOMEM);
> +
> +       /*
> +        * If battery info is not supplied just ignore and program default
> +        * values.
> +        */
> +       power_supply_get_battery_info(charger->psy, &info);
> +
> +       if (info.charge_voltage_uv > 0)
> +               pdata->charge_voltage_uv = info.charge_voltage_uv;
> +       else
> +               pdata->charge_voltage_uv = 4100000;
> +

Maybe dev.warn() here if input won't take effect due to incorrect
value, and leave pdata->* in a valid state, instead of checking input
in set_charge_*(). That also makes get_property() correct in all cases
(alternatively get_property should obtain values from chip).

> +       if (info.charge_current_ua > 0)
> +               pdata->charge_current_ua = info.charge_current_ua;
> +       else
> +               pdata->charge_current_ua = 500000;
> +

Same here.

> +       ret = device_property_read_u32(charger->dev, "ti,ntc-type",
> +                                      &pdata->ntc_type);
> +       if (ret)
> +               pdata->ntc_type = 1;    /* 10k  (curve 2, B = 3480) */
> +
> +       return pdata;
> +}
> +#else /* CONFIG_OF */
> +static struct tps65217_charger_platform_data *tps65217_charger_pdata_init(
> +               struct tps65217_charger *charger)
> +{
> +       return NULL;
> +}
> +#endif /* CONFIG_OF */
> +
>  static const struct power_supply_desc tps65217_charger_desc = {
>         .name                   = "tps65217-charger",
>         .type                   = POWER_SUPPLY_TYPE_MAINS,
> @@ -222,8 +394,11 @@ static int tps65217_charger_probe(struct platform_device *pdev)
>                 return PTR_ERR(charger->psy);
>         }
>
> -       irq[0] = platform_get_irq_byname(pdev, "USB");
> -       irq[1] = platform_get_irq_byname(pdev, "AC");
> +       charger->pdata = tps65217_charger_pdata_init(charger);
> +       if (IS_ERR(charger->pdata)) {
> +               dev_err(charger->dev, "failed: getting platform data\n");
> +               return PTR_ERR(charger->pdata);
> +       }
>
>         ret = tps65217_config_charger(charger);
>         if (ret < 0) {
> @@ -231,6 +406,9 @@ static int tps65217_charger_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       irq[0] = platform_get_irq_byname(pdev, "USB");
> +       irq[1] = platform_get_irq_byname(pdev, "AC");
> +
>         /* Create a polling thread if an interrupt is invalid */
>         if (irq[0] < 0 || irq[1] < 0) {
>                 poll_task = kthread_run(tps65217_charger_poll_task,
> diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
> index eac2857..d040062 100644
> --- a/include/linux/mfd/tps65217.h
> +++ b/include/linux/mfd/tps65217.h
> @@ -103,8 +103,10 @@
>  #define TPS65217_CHGCONFIG2_DYNTMR     BIT(7)
>  #define TPS65217_CHGCONFIG2_VPREGHG    BIT(6)
>  #define TPS65217_CHGCONFIG2_VOREG_MASK 0x30
> +#define TPS65217_CHGCONFIG2_VOREG_SHIFT        4
>
>  #define TPS65217_CHGCONFIG3_ICHRG_MASK 0xC0
> +#define TPS65217_CHGCONFIG3_ICHRG_SHIFT        6
>  #define TPS65217_CHGCONFIG3_DPPMTH_MASK        0x30
>  #define TPS65217_CHGCONFIG2_PCHRGT     BIT(3)
>  #define TPS65217_CHGCONFIG2_TERMIF     0x06
> --
> 2.9.3
>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Enric Balletbo Serra]

Hi Liam,

2017-05-26 23:20 GMT+02:00 Liam Breck <liam@networkimprov.net>:
> Hi Enric,
>
> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>> Add charging voltage and current characteristics to the battery DT for
>> proper handling of the battery by fuel-gauge and charger chips.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> Changes since v2:
>>  - Requested by Sebastian Reichel
>>    - Move to its own patch and apply to simple-battery framework.
>> Changes since v1:
>>  - Requested by Rob Herring
>>    - Rename ti,charge-* to charge-* to be standard properties.
>>    - Use unit suffixes as per bindings/property-units.txt
>>
>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>> index 63a7028..c87a439 100644
>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>> @@ -12,6 +12,8 @@ Optional Properties:
>>   - voltage-min-design-microvolt: drained battery voltage
>>   - energy-full-design-microwatt-hours: battery design energy
>>   - charge-full-design-microamp-hours: battery design capacity
>> + - charge-voltage-microvolt: battery charging voltage
>> + - charge-current-microamp: battery charging current
>
> I think you mean constant-charge-* which is how you surface these
> properties in your tps65217_charger patch.
>

Yes, to be strict this is constant-charge-*

> I'll add these to v14 of my patchset which adds simple-battery
> support. Rob requested a single patch for this file.
>

Ok, I'll send the tps charger series without this patch, so please,
include this patch in your series.

> I've been waiting for feedback on v13.2 from Sebastian. If I don't
> hear from him within a few days, I'll post v14.
>
>>  Batteries must be referenced by chargers and/or fuel-gauges
>>  using a phandle. The phandle's property should be named
>> @@ -24,6 +26,8 @@ Example:
>>                 voltage-min-design-microvolt = <3200000>;
>>                 energy-full-design-microwatt-hours = <5290000>;
>>                 charge-full-design-microamp-hours = <1430000>;
>> +               charge-voltage-microvolt = <4100000>;
>> +               charge-current-microamp = <300000>;
>>         };
>>
>>         charger: charger@11 {
>> --
>> 2.9.3
>>

Re: [PATCH v3 2/4] power: supply: core: add charging voltage/current battery info

[Enric Balletbo Serra]

Hi Liam,

2017-05-26 23:22 GMT+02:00 Liam Breck <liam@networkimprov.net>:
> Hi Enric,
>
> I'll also incorporate these changes into v14 of my patchset. (See my prev msg.)
>

Perfect, thanks


>
> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>> Add the parameters to define the battery charging voltage and charging
>> current. Charger driver can get this information from the struct
>> power_supply_battery_info and apply the desired value.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> Changes since v2:
>>  - Requested by Sebastian Reichel
>>    - Move to its own patch and apply to simple-battery framework.
>> Changes since v1:
>>  - Requested by Rob Herring
>>    - Rename ti,charge-* to charge-* to be standard properties.
>>    - Use unit suffixes as per bindings/property-units.txt
>>
>>  drivers/power/supply/power_supply_core.c | 6 ++++++
>>  include/linux/power_supply.h             | 2 ++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index 862fa8fc..a6857c2 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -530,6 +530,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
>>         info->energy_full_design_uwh = -EINVAL;
>>         info->charge_full_design_uah = -EINVAL;
>>         info->voltage_min_design_uv  = -EINVAL;
>> +       info->charge_voltage_uv = -EINVAL;
>> +       info->charge_current_ua = -EINVAL;
>>
>>         if (!psy->of_node) {
>>                 dev_warn(&psy->dev, "%s currently only supports devicetree\n",
>> @@ -559,6 +561,10 @@ int power_supply_get_battery_info(struct power_supply *psy,
>>                              &info->charge_full_design_uah);
>>         of_property_read_u32(battery_np, "voltage-min-design-microvolt",
>>                              &info->voltage_min_design_uv);
>> +       of_property_read_u32(battery_np, "charge-voltage-microvolt",
>> +                            &info->charge_voltage_uv);
>> +       of_property_read_u32(battery_np, "charge-current-microamp",
>> +                            &info->charge_current_ua);
>>
>>         return 0;
>>  }
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index 8220f7b..3eea323 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -302,6 +302,8 @@ struct power_supply_battery_info {
>>         int energy_full_design_uwh;     /* microWatt-hours */
>>         int charge_full_design_uah;     /* microAmp-hours */
>>         int voltage_min_design_uv;      /* microVolts */
>> +       int charge_voltage_uv;          /* microVolts */
>> +       int charge_current_ua;          /* microAmp */
>>  };
>>
>>  extern struct atomic_notifier_head power_supply_notifier;
>> --
>> 2.9.3
>>

Re: [PATCH v3 3/4] dt-bindings: power: tps65217_charger: add NTC type for battery temperature measurement.

[Enric Balletbo Serra]

2017-05-26 23:26 GMT+02:00 Liam Breck <liam@networkimprov.net>:
> Hi Enric,
>
> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>> The TS pin of the TPS56217 connects to the NTC resistor in the battery
>> pack. By default the device is setup to support a 10-kohm but can also
>> be configured to support a 100-kohm. Add a propietry to configure the
>> connected NTC resistor. Therefore, the charger would get the wrong
>> temperature information.
>
> This also needs a monitored-battery property, and a battery node in
> the example. See https://patchwork.kernel.org/patch/9710803/
>

Right I'll add the property in v4.

>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> Changes since v2:
>>  - Requested by Sebastian Reichel
>>    - Get rid of common properties and only maintain ntc-type.
>> Changes since v1:
>>  - None
>>
>>  .../devicetree/bindings/power/supply/tps65217_charger.txt          | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
>> index a11072c..851f5c7 100644
>> --- a/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
>> @@ -6,6 +6,12 @@ Required Properties:
>>               Should be <0> for the USB charger and <1> for the AC adapter.
>>  -interrupt-names: Should be "USB" and "AC"
>>
>> +Optional properties:
>> +-ti,ntc-type: set the NTC type for battery temperature measurement. The value
>> +       must be 0 or 1, where:
>> +         0 – 100k (curve 1, B = 3960)
>> +         1 – 10k  (curve 2, B = 3480) (default)
>> +
>>  This node is a subnode of the tps65217 PMIC.
>>
>>  Example:
>> @@ -14,4 +20,5 @@ Example:
>>                 compatible = "ti,tps65217-charger";
>>                 interrupts = <0>, <1>;
>>                 interrupt-names = "USB", "AC";
>> +               ti,ntc-type = <1>;
>>         };
>> --
>> 2.9.3
>>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Rob Herring]

On Fri, May 26, 2017 at 01:04:10PM +0200, Enric Balletbo i Serra wrote:
> Add charging voltage and current characteristics to the battery DT for
> proper handling of the battery by fuel-gauge and charger chips.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>    - Move to its own patch and apply to simple-battery framework.
> Changes since v1:
>  - Requested by Rob Herring
>    - Rename ti,charge-* to charge-* to be standard properties.
>    - Use unit suffixes as per bindings/property-units.txt
> 
>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,

On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> Hi Liam,
>
> 2017-05-26 23:20 GMT+02:00 Liam Breck <liam@networkimprov.net>:
>> Hi Enric,
>>
>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>> <enric.balletbo@collabora.com> wrote:
>>> Add charging voltage and current characteristics to the battery DT for
>>> proper handling of the battery by fuel-gauge and charger chips.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> ---
>>> Changes since v2:
>>>  - Requested by Sebastian Reichel
>>>    - Move to its own patch and apply to simple-battery framework.
>>> Changes since v1:
>>>  - Requested by Rob Herring
>>>    - Rename ti,charge-* to charge-* to be standard properties.
>>>    - Use unit suffixes as per bindings/property-units.txt
>>>
>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>>> index 63a7028..c87a439 100644
>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>   - voltage-min-design-microvolt: drained battery voltage
>>>   - energy-full-design-microwatt-hours: battery design energy
>>>   - charge-full-design-microamp-hours: battery design capacity
>>> + - charge-voltage-microvolt: battery charging voltage
>>> + - charge-current-microamp: battery charging current
>>
>> I think you mean constant-charge-* which is how you surface these
>> properties in your tps65217_charger patch.
>>
>
> Yes, to be strict this is constant-charge-*

The DT battery node should carry static battery characteristics. So on
reflection, I think you want

constant-charge-current-max-microamp
constant-charge-voltage-max-microvolt

The charger or the user could then safely apply any value <= those.

Thoughts?

>> I'll add these to v14 of my patchset which adds simple-battery
>> support. Rob requested a single patch for this file.
>>
>
> Ok, I'll send the tps charger series without this patch, so please,
> include this patch in your series.
>
>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>> hear from him within a few days, I'll post v14.
>>
>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>  using a phandle. The phandle's property should be named
>>> @@ -24,6 +26,8 @@ Example:
>>>                 voltage-min-design-microvolt = <3200000>;
>>>                 energy-full-design-microwatt-hours = <5290000>;
>>>                 charge-full-design-microamp-hours = <1430000>;
>>> +               charge-voltage-microvolt = <4100000>;
>>> +               charge-current-microamp = <300000>;
>>>         };
>>>
>>>         charger: charger@11 {
>>> --
>>> 2.9.3
>>>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Enric Balletbo Serra]

Hi Liam,

2017-06-01 9:01 GMT+02:00 Liam Breck <liam@networkimprov.net>:
> Hi Enric,
>
> On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
>> Hi Liam,
>>
>> 2017-05-26 23:20 GMT+02:00 Liam Breck <liam@networkimprov.net>:
>>> Hi Enric,
>>>
>>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>>> <enric.balletbo@collabora.com> wrote:
>>>> Add charging voltage and current characteristics to the battery DT for
>>>> proper handling of the battery by fuel-gauge and charger chips.
>>>>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>> Changes since v2:
>>>>  - Requested by Sebastian Reichel
>>>>    - Move to its own patch and apply to simple-battery framework.
>>>> Changes since v1:
>>>>  - Requested by Rob Herring
>>>>    - Rename ti,charge-* to charge-* to be standard properties.
>>>>    - Use unit suffixes as per bindings/property-units.txt
>>>>
>>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>> index 63a7028..c87a439 100644
>>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>>   - voltage-min-design-microvolt: drained battery voltage
>>>>   - energy-full-design-microwatt-hours: battery design energy
>>>>   - charge-full-design-microamp-hours: battery design capacity
>>>> + - charge-voltage-microvolt: battery charging voltage
>>>> + - charge-current-microamp: battery charging current
>>>
>>> I think you mean constant-charge-* which is how you surface these
>>> properties in your tps65217_charger patch.
>>>
>>
>> Yes, to be strict this is constant-charge-*
>
> The DT battery node should carry static battery characteristics. So on
> reflection, I think you want
>
> constant-charge-current-max-microamp
> constant-charge-voltage-max-microvolt
>
> The charger or the user could then safely apply any value <= those.
>
> Thoughts?
>

Hmm I see your point and I'm thinking now that actually this is not
what I wanted to do. What I wanted is set the charger voltage and the
current hence my first patchset was setting these properties in the
charger node not the battery.

Said that, I think that you have reason and what we want in battery
node is the current/voltage max values but we also need to implement a
mechanism to set the charging voltage/current from userspace or from
the DT for the charger. The charger should be able to set these values
and fail if, based in the battery specs, is not supported.

More thoughts?

>>> I'll add these to v14 of my patchset which adds simple-battery
>>> support. Rob requested a single patch for this file.
>>>
>>
>> Ok, I'll send the tps charger series without this patch, so please,
>> include this patch in your series.
>>
>>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>>> hear from him within a few days, I'll post v14.
>>>
>>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>>  using a phandle. The phandle's property should be named
>>>> @@ -24,6 +26,8 @@ Example:
>>>>                 voltage-min-design-microvolt = <3200000>;
>>>>                 energy-full-design-microwatt-hours = <5290000>;
>>>>                 charge-full-design-microamp-hours = <1430000>;
>>>> +               charge-voltage-microvolt = <4100000>;
>>>> +               charge-current-microamp = <300000>;
>>>>         };
>>>>
>>>>         charger: charger@11 {
>>>> --
>>>> 2.9.3
>>>>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Liam Breck]

Hi Enric,



On Thu, Jun 1, 2017 at 1:18 AM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> Hi Liam,
>
> 2017-06-01 9:01 GMT+02:00 Liam Breck <liam@networkimprov.net>:
>> Hi Enric,
>>
>> On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
>> <eballetbo@gmail.com> wrote:
>>> Hi Liam,
>>>
>>> 2017-05-26 23:20 GMT+02:00 Liam Breck <liam@networkimprov.net>:
>>>> Hi Enric,
>>>>
>>>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>>>> <enric.balletbo@collabora.com> wrote:
>>>>> Add charging voltage and current characteristics to the battery DT for
>>>>> proper handling of the battery by fuel-gauge and charger chips.
>>>>>
>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>>> ---
>>>>> Changes since v2:
>>>>>  - Requested by Sebastian Reichel
>>>>>    - Move to its own patch and apply to simple-battery framework.
>>>>> Changes since v1:
>>>>>  - Requested by Rob Herring
>>>>>    - Rename ti,charge-* to charge-* to be standard properties.
>>>>>    - Use unit suffixes as per bindings/property-units.txt
>>>>>
>>>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> index 63a7028..c87a439 100644
>>>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>>>   - voltage-min-design-microvolt: drained battery voltage
>>>>>   - energy-full-design-microwatt-hours: battery design energy
>>>>>   - charge-full-design-microamp-hours: battery design capacity
>>>>> + - charge-voltage-microvolt: battery charging voltage
>>>>> + - charge-current-microamp: battery charging current
>>>>
>>>> I think you mean constant-charge-* which is how you surface these
>>>> properties in your tps65217_charger patch.
>>>>
>>>
>>> Yes, to be strict this is constant-charge-*
>>
>> The DT battery node should carry static battery characteristics. So on
>> reflection, I think you want
>>
>> constant-charge-current-max-microamp
>> constant-charge-voltage-max-microvolt
>>
>> The charger or the user could then safely apply any value <= those.
>>
>> Thoughts?

I'm curious to hear how your hw config requires constant-charge settings?

> Hmm I see your point and I'm thinking now that actually this is not
> what I wanted to do. What I wanted is set the charger voltage and the
> current hence my first patchset was setting these properties in the
> charger node not the battery.

You could certainly support constant-charge-* params in the charger
node. And it's easy to enable userspace to set these via sysfs.

> Said that, I think that you have reason and what we want in battery
> node is the current/voltage max values but we also need to implement a
> mechanism to set the charging voltage/current from userspace or from
> the DT for the charger. The charger should be able to set these values
> and fail if, based in the battery specs, is not supported.

The charger could default constant-charge-* to the battery node's max values.

See also this discussion about similar issues.
https://patchwork.kernel.org/patch/9625331/


> More thoughts?
>
>>>> I'll add these to v14 of my patchset which adds simple-battery
>>>> support. Rob requested a single patch for this file.
>>>>
>>>
>>> Ok, I'll send the tps charger series without this patch, so please,
>>> include this patch in your series.
>>>
>>>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>>>> hear from him within a few days, I'll post v14.
>>>>
>>>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>>>  using a phandle. The phandle's property should be named
>>>>> @@ -24,6 +26,8 @@ Example:
>>>>>                 voltage-min-design-microvolt = <3200000>;
>>>>>                 energy-full-design-microwatt-hours = <5290000>;
>>>>>                 charge-full-design-microamp-hours = <1430000>;
>>>>> +               charge-voltage-microvolt = <4100000>;
>>>>> +               charge-current-microamp = <300000>;
>>>>>         };
>>>>>
>>>>>         charger: charger@11 {
>>>>> --
>>>>> 2.9.3
>>>>>

Re: [PATCH v3 1/4] dt-bindings: power: supply: add battery charge voltage/current

[Enric Balletbo Serra]

2017-06-01 12:25 GMT+02:00 Liam Breck <liam@networkimprov.net>:
> Hi Enric,
>
>
>
> On Thu, Jun 1, 2017 at 1:18 AM, Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
>> Hi Liam,
>>
>> 2017-06-01 9:01 GMT+02:00 Liam Breck <liam@networkimprov.net>:
>>> Hi Enric,
>>>
>>> On Sat, May 27, 2017 at 1:11 PM, Enric Balletbo Serra
>>> <eballetbo@gmail.com> wrote:
>>>> Hi Liam,
>>>>
>>>> 2017-05-26 23:20 GMT+02:00 Liam Breck <liam@networkimprov.net>:
>>>>> Hi Enric,
>>>>>
>>>>> On Fri, May 26, 2017 at 4:04 AM, Enric Balletbo i Serra
>>>>> <enric.balletbo@collabora.com> wrote:
>>>>>> Add charging voltage and current characteristics to the battery DT for
>>>>>> proper handling of the battery by fuel-gauge and charger chips.
>>>>>>
>>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>>>> ---
>>>>>> Changes since v2:
>>>>>>  - Requested by Sebastian Reichel
>>>>>>    - Move to its own patch and apply to simple-battery framework.
>>>>>> Changes since v1:
>>>>>>  - Requested by Rob Herring
>>>>>>    - Rename ti,charge-* to charge-* to be standard properties.
>>>>>>    - Use unit suffixes as per bindings/property-units.txt
>>>>>>
>>>>>>  Documentation/devicetree/bindings/power/supply/battery.txt | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>> index 63a7028..c87a439 100644
>>>>>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>> @@ -12,6 +12,8 @@ Optional Properties:
>>>>>>   - voltage-min-design-microvolt: drained battery voltage
>>>>>>   - energy-full-design-microwatt-hours: battery design energy
>>>>>>   - charge-full-design-microamp-hours: battery design capacity
>>>>>> + - charge-voltage-microvolt: battery charging voltage
>>>>>> + - charge-current-microamp: battery charging current
>>>>>
>>>>> I think you mean constant-charge-* which is how you surface these
>>>>> properties in your tps65217_charger patch.
>>>>>
>>>>
>>>> Yes, to be strict this is constant-charge-*
>>>
>>> The DT battery node should carry static battery characteristics. So on
>>> reflection, I think you want
>>>
>>> constant-charge-current-max-microamp
>>> constant-charge-voltage-max-microvolt
>>>
>>> The charger or the user could then safely apply any value <= those.
>>>
>>> Thoughts?
>
> I'm curious to hear how your hw config requires constant-charge settings?
>
>> Hmm I see your point and I'm thinking now that actually this is not
>> what I wanted to do. What I wanted is set the charger voltage and the
>> current hence my first patchset was setting these properties in the
>> charger node not the battery.
>
> You could certainly support constant-charge-* params in the charger
> node. And it's easy to enable userspace to set these via sysfs.
>
>> Said that, I think that you have reason and what we want in battery
>> node is the current/voltage max values but we also need to implement a
>> mechanism to set the charging voltage/current from userspace or from
>> the DT for the charger. The charger should be able to set these values
>> and fail if, based in the battery specs, is not supported.
>
> The charger could default constant-charge-* to the battery node's max values.
>
> See also this discussion about similar issues.
> https://patchwork.kernel.org/patch/9625331/
>

Oh I missed that thread, please keep me in the loop for next versions
of the patchset :)

Best regards,
 Enric

>
>> More thoughts?
>>
>>>>> I'll add these to v14 of my patchset which adds simple-battery
>>>>> support. Rob requested a single patch for this file.
>>>>>
>>>>
>>>> Ok, I'll send the tps charger series without this patch, so please,
>>>> include this patch in your series.
>>>>
>>>>> I've been waiting for feedback on v13.2 from Sebastian. If I don't
>>>>> hear from him within a few days, I'll post v14.
>>>>>
>>>>>>  Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>  using a phandle. The phandle's property should be named
>>>>>> @@ -24,6 +26,8 @@ Example:
>>>>>>                 voltage-min-design-microvolt = <3200000>;
>>>>>>                 energy-full-design-microwatt-hours = <5290000>;
>>>>>>                 charge-full-design-microamp-hours = <1430000>;
>>>>>> +               charge-voltage-microvolt = <4100000>;
>>>>>> +               charge-current-microamp = <300000>;
>>>>>>         };
>>>>>>
>>>>>>         charger: charger@11 {
>>>>>> --
>>>>>> 2.9.3
>>>>>>

Re: [PATCH v3 4/4] power: tps65217_charger: add support for NTC type, voltage and current charge

[Sebastian Reichel]

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

Hi Enric,

On Fri, May 26, 2017 at 01:04:13PM +0200, Enric Balletbo i Serra wrote:
> Allow the possibility to configure the charge and the current voltage of
> the charger and also the NTC type for battery temperature measurement.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v2:
>  - Requested by Sebastian Reichel
>    - Use the simple-battery framework
>    - Use device_property_read_u32 instead of of_property_read_u32
> Changes since v1:
>  - None
> 
>  drivers/power/supply/tps65217_charger.c | 194 ++++++++++++++++++++++++++++++--
>  include/linux/mfd/tps65217.h            |   2 +
>  2 files changed, 188 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
> index 1f52340..5939e77 100644
> --- a/drivers/power/supply/tps65217_charger.c
> +++ b/drivers/power/supply/tps65217_charger.c
>
> [...]
>
> +#ifdef CONFIG_OF

You can drop the #ifdef CONFIG_OF. device_properties can also be
added via ACPI or boardcode and power_supply_get_battery_info is
always available (but will return -EINVAL values for !OF at the
moment).

> +static struct tps65217_charger_platform_data *tps65217_charger_pdata_init(
> +		struct tps65217_charger *charger)
> +{
> +	struct tps65217_charger_platform_data *pdata;
> +	struct power_supply_battery_info info = {};
> +	int ret;
> +
> +	pdata = devm_kzalloc(charger->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/*
> +	 * If battery info is not supplied just ignore and program default
> +	 * values.
> +	 */
> +	power_supply_get_battery_info(charger->psy, &info);
> +
> +	if (info.charge_voltage_uv > 0)
> +		pdata->charge_voltage_uv = info.charge_voltage_uv;
> +	else
> +		pdata->charge_voltage_uv = 4100000;
> +
> +	if (info.charge_current_ua > 0)
> +		pdata->charge_current_ua = info.charge_current_ua;
> +	else
> +		pdata->charge_current_ua = 500000;
> +
> +	ret = device_property_read_u32(charger->dev, "ti,ntc-type",
> +				       &pdata->ntc_type);
> +	if (ret)
> +		pdata->ntc_type = 1;	/* 10k  (curve 2, B = 3480) */
> +
> +	return pdata;
> +}
> +#else /* CONFIG_OF */
> +static struct tps65217_charger_platform_data *tps65217_charger_pdata_init(
> +		struct tps65217_charger *charger)
> +{
> +	return NULL;
> +}
> +#endif /* CONFIG_OF */
> +
> [...]
>

Otherwise looks fine to me.

-- Sebastian

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