[PATCH v4 0/6] Convert twl4030_madc_battery to IIO consumer and add DT aupport

[PATCH v4 0/6] Convert twl4030_madc_battery to IIO consumer and add DT aupport

[Marek Belisko]

This patches adding support for twl4030_madc_battery to use twl4030_madc iio framework + DT support.
Patches was tested on gta04 board. twl4030_madc_battery driver is converted in
first patch to iio consumer and in next patches is added support for devicetree
+ some small fixes for module autoloading.

Changes from V3:
- added ti prefix for volt-to-capacity-discharging/charging-map properties

Changes from V2:
- fix property names
- add MODULE_DEVICE_TABLE and MODULE_ALIAS

Changes from V1:
- use iio_read_channel_processed instead iio_read_channel_processed
- convert probe to use devm_ as part of adding error handling for iio
- free iio channels on error and on module removal

[1] - https://lkml.org/lkml/2014/2/26/482dd

Marek Belisko (6):
  power: twl4030-madc-battery: Convert to iio consumer.
  power: twl4030_madc_battery: Add device tree support
  Documentation: DT: Document twl4030-madc-battery bindings
  ARM: dts: omap3-gta04: Add battery support
  power: twl4030_madc_battery: Add of_twl4030_madc_match to
    MODULE_DEVICE_TABLE
  power: twl4030_madc_battery: Add missing MODULE_ALIAS

 .../bindings/power_supply/twl4030_madc_battery.txt |  43 +++++
 arch/arm/boot/dts/omap3-gta04.dtsi                 |  30 ++++
 drivers/power/twl4030_madc_battery.c               | 183 +++++++++++++++++----
 3 files changed, 221 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt

-- 
1.9.1

[PATCH v4 1/6] power: twl4030-madc-battery: Convert to iio consumer.

[Marek Belisko]

Because of added iio error handling private data allocation was converted
to managed to simplify code.

Signed-off-by: Marek Belisko <marek@goldelico.com>
Reviewed-By: Sebastian Reichel <sre@debian.org>
---
 drivers/power/twl4030_madc_battery.c | 99 +++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 35 deletions(-)

diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
index 7ef445a..6af28b5 100644
--- a/drivers/power/twl4030_madc_battery.c
+++ b/drivers/power/twl4030_madc_battery.c
@@ -19,10 +19,14 @@
 #include <linux/sort.h>
 #include <linux/i2c/twl4030-madc.h>
 #include <linux/power/twl4030_madc_battery.h>
+#include <linux/iio/consumer.h>
 
 struct twl4030_madc_battery {
 	struct power_supply psy;
 	struct twl4030_madc_bat_platform_data *pdata;
+	struct iio_channel *channel_temp;
+	struct iio_channel *channel_ichg;
+	struct iio_channel *channel_vbat;
 };
 
 static enum power_supply_property twl4030_madc_bat_props[] = {
@@ -38,43 +42,34 @@ static enum power_supply_property twl4030_madc_bat_props[] = {
 	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
 };
 
-static int madc_read(int index)
+static int madc_read(struct iio_channel *channel)
 {
-	struct twl4030_madc_request req;
-	int val;
+	int val, err;
+	err = iio_read_channel_processed(channel, &val);
+	if (err < 0)
+		return err;
 
-	req.channels = index;
-	req.method = TWL4030_MADC_SW2;
-	req.type = TWL4030_MADC_WAIT;
-	req.do_avg = 0;
-	req.raw = false;
-	req.func_cb = NULL;
-
-	val = twl4030_madc_conversion(&req);
-	if (val < 0)
-		return val;
-
-	return req.rbuf[ffs(index) - 1];
+	return val;
 }
 
-static int twl4030_madc_bat_get_charging_status(void)
+static int twl4030_madc_bat_get_charging_status(struct twl4030_madc_battery *bt)
 {
-	return (madc_read(TWL4030_MADC_ICHG) > 0) ? 1 : 0;
+	return (madc_read(bt->channel_ichg) > 0) ? 1 : 0;
 }
 
-static int twl4030_madc_bat_get_voltage(void)
+static int twl4030_madc_bat_get_voltage(struct twl4030_madc_battery *bt)
 {
-	return madc_read(TWL4030_MADC_VBAT);
+	return madc_read(bt->channel_vbat);
 }
 
-static int twl4030_madc_bat_get_current(void)
+static int twl4030_madc_bat_get_current(struct twl4030_madc_battery *bt)
 {
-	return madc_read(TWL4030_MADC_ICHG) * 1000;
+	return madc_read(bt->channel_ichg) * 1000;
 }
 
-static int twl4030_madc_bat_get_temp(void)
+static int twl4030_madc_bat_get_temp(struct twl4030_madc_battery *bt)
 {
-	return madc_read(TWL4030_MADC_BTEMP) * 10;
+	return madc_read(bt->channel_temp) * 10;
 }
 
 static int twl4030_madc_bat_voltscale(struct twl4030_madc_battery *bat,
@@ -84,7 +79,7 @@ static int twl4030_madc_bat_voltscale(struct twl4030_madc_battery *bat,
 	int i, res = 0;
 
 	/* choose charging curve */
-	if (twl4030_madc_bat_get_charging_status())
+	if (twl4030_madc_bat_get_charging_status(bat))
 		calibration = bat->pdata->charging;
 	else
 		calibration = bat->pdata->discharging;
@@ -119,23 +114,23 @@ static int twl4030_madc_bat_get_property(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
 		if (twl4030_madc_bat_voltscale(bat,
-				twl4030_madc_bat_get_voltage()) > 95)
+				twl4030_madc_bat_get_voltage(bat)) > 95)
 			val->intval = POWER_SUPPLY_STATUS_FULL;
 		else {
-			if (twl4030_madc_bat_get_charging_status())
+			if (twl4030_madc_bat_get_charging_status(bat))
 				val->intval = POWER_SUPPLY_STATUS_CHARGING;
 			else
 				val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		}
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = twl4030_madc_bat_get_voltage() * 1000;
+		val->intval = twl4030_madc_bat_get_voltage(bat) * 1000;
 		break;
 	case POWER_SUPPLY_PROP_TECHNOLOGY:
 		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
-		val->intval = twl4030_madc_bat_get_current();
+		val->intval = twl4030_madc_bat_get_current(bat);
 		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		/* assume battery is always present */
@@ -143,23 +138,23 @@ static int twl4030_madc_bat_get_property(struct power_supply *psy,
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_NOW: {
 			int percent = twl4030_madc_bat_voltscale(bat,
-					twl4030_madc_bat_get_voltage());
+					twl4030_madc_bat_get_voltage(bat));
 			val->intval = (percent * bat->pdata->capacity) / 100;
 			break;
 		}
 	case POWER_SUPPLY_PROP_CAPACITY:
 		val->intval = twl4030_madc_bat_voltscale(bat,
-					twl4030_madc_bat_get_voltage());
+					twl4030_madc_bat_get_voltage(bat));
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 		val->intval = bat->pdata->capacity;
 		break;
 	case POWER_SUPPLY_PROP_TEMP:
-		val->intval = twl4030_madc_bat_get_temp();
+		val->intval = twl4030_madc_bat_get_temp(bat);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW: {
 			int percent = twl4030_madc_bat_voltscale(bat,
-					twl4030_madc_bat_get_voltage());
+					twl4030_madc_bat_get_voltage(bat));
 			/* in mAh */
 			int chg = (percent * (bat->pdata->capacity/1000))/100;
 
@@ -192,8 +187,10 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
 {
 	struct twl4030_madc_battery *twl4030_madc_bat;
 	struct twl4030_madc_bat_platform_data *pdata = pdev->dev.platform_data;
+	int ret;
 
-	twl4030_madc_bat = kzalloc(sizeof(*twl4030_madc_bat), GFP_KERNEL);
+	twl4030_madc_bat = devm_kzalloc(&pdev->dev, sizeof(*twl4030_madc_bat),
+				GFP_KERNEL);
 	if (!twl4030_madc_bat)
 		return -ENOMEM;
 
@@ -206,6 +203,24 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
 	twl4030_madc_bat->psy.external_power_changed =
 					twl4030_madc_bat_ext_changed;
 
+	twl4030_madc_bat->channel_temp = iio_channel_get(&pdev->dev, "temp");
+	if (IS_ERR(twl4030_madc_bat->channel_temp)) {
+		ret = PTR_ERR(twl4030_madc_bat->channel_temp);
+		goto err;
+	}
+
+	twl4030_madc_bat->channel_ichg  = iio_channel_get(&pdev->dev, "ichg");
+	if (IS_ERR(twl4030_madc_bat->channel_ichg)) {
+		ret = PTR_ERR(twl4030_madc_bat->channel_ichg);
+		goto err_temp;
+	}
+
+	twl4030_madc_bat->channel_vbat = iio_channel_get(&pdev->dev, "vbat");
+	if (IS_ERR(twl4030_madc_bat->channel_vbat)) {
+		ret = PTR_ERR(twl4030_madc_bat->channel_vbat);
+		goto err_ichg;
+	}
+
 	/* sort charging and discharging calibration data */
 	sort(pdata->charging, pdata->charging_size,
 		sizeof(struct twl4030_madc_bat_calibration),
@@ -216,9 +231,20 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
 
 	twl4030_madc_bat->pdata = pdata;
 	platform_set_drvdata(pdev, twl4030_madc_bat);
-	power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
+	ret = power_supply_register(&pdev->dev, &twl4030_madc_bat->psy);
+	if (ret)
+		goto err_vbat;
 
 	return 0;
+
+err_vbat:
+	iio_channel_release(twl4030_madc_bat->channel_vbat);
+err_ichg:
+	iio_channel_release(twl4030_madc_bat->channel_ichg);
+err_temp:
+	iio_channel_release(twl4030_madc_bat->channel_temp);
+err:
+	return ret;
 }
 
 static int twl4030_madc_battery_remove(struct platform_device *pdev)
@@ -226,7 +252,10 @@ static int twl4030_madc_battery_remove(struct platform_device *pdev)
 	struct twl4030_madc_battery *bat = platform_get_drvdata(pdev);
 
 	power_supply_unregister(&bat->psy);
-	kfree(bat);
+
+	iio_channel_release(bat->channel_vbat);
+	iio_channel_release(bat->channel_ichg);
+	iio_channel_release(bat->channel_temp);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH v4 1/6] power: twl4030-madc-battery: Convert to iio consumer.

[Sebastian Reichel]

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

Hi,

On Tue, Mar 10, 2015 at 10:27:22PM +0100, Marek Belisko wrote:
> Because of added iio error handling private data allocation was
> converted to managed to simplify code.

Thanks, pulled (only PATCH 1/6 for now).

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v4 2/6] power: twl4030_madc_battery: Add device tree support

[Marek Belisko]

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 drivers/power/twl4030_madc_battery.c | 81 ++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
index 6af28b5..4bcb4a9 100644
--- a/drivers/power/twl4030_madc_battery.c
+++ b/drivers/power/twl4030_madc_battery.c
@@ -20,6 +20,7 @@
 #include <linux/i2c/twl4030-madc.h>
 #include <linux/power/twl4030_madc_battery.h>
 #include <linux/iio/consumer.h>
+#include <linux/of.h>
 
 struct twl4030_madc_battery {
 	struct power_supply psy;
@@ -183,6 +184,82 @@ static int twl4030_cmp(const void *a, const void *b)
 		((struct twl4030_madc_bat_calibration *)a)->voltage;
 }
 
+#ifdef CONFIG_OF
+static struct twl4030_madc_bat_platform_data *
+	twl4030_madc_dt_probe(struct platform_device *pdev)
+{
+	struct twl4030_madc_bat_platform_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+	int i, proplen;
+
+	pdata = devm_kzalloc(&pdev->dev,
+			sizeof(struct twl4030_madc_bat_platform_data),
+			GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	ret = of_property_read_u32(np, "capacity-uah", &pdata->capacity);
+	if (ret != 0)
+		return ERR_PTR(-EINVAL);
+
+	/* parse and prepare charging data */
+	proplen = of_property_count_u32_elems(np, "ti,volt-to-capacity-charging-map");
+	if (proplen < 0) {
+		dev_warn(&pdev->dev, "No 'ti,volt-to-capacity-charging-map' property found\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	pdata->charging = devm_kzalloc(&pdev->dev,
+		sizeof(struct twl4030_madc_bat_calibration) * (proplen / 2),
+		GFP_KERNEL);
+
+	for (i = 0; i < proplen / 2; i++) {
+		of_property_read_u32_index(np, "ti,volt-to-capacity-charging-map",
+					   i * 2,
+					   (u32 *)&pdata->charging[i].voltage);
+		of_property_read_u32_index(np, "ti,volt-to-capacity-charging-map",
+					  i * 2 + 1,
+					  (u32 *)&pdata->charging[i].level);
+	}
+
+	/* parse and prepare discharging data */
+	proplen = of_property_count_u32_elems(np,
+			"ti,volt-to-capacity-discharging-map");
+	if (proplen < 0) {
+		dev_warn(&pdev->dev, "No 'ti,volt-to-capacity-discharging-map' property found\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	pdata->discharging = devm_kzalloc(&pdev->dev,
+		sizeof(struct twl4030_madc_bat_calibration) * (proplen / 2),
+		GFP_KERNEL);
+
+	for (i = 0; i < proplen / 2; i++) {
+		of_property_read_u32_index(np, "ti,volt-to-capacity-discharging-map",
+					   i * 2,
+					   (u32 *)&pdata->discharging[i].voltage);
+		of_property_read_u32_index(np, "ti,volt-to-capacity-discharging-map",
+					   i * 2 + 1,
+					   (u32 *)&pdata->discharging[i].level);
+	}
+
+	return pdata;
+}
+
+static const struct of_device_id of_twl4030_madc_match[] = {
+	{ .compatible = "ti,twl4030-madc-battery", },
+	{},
+};
+
+#else
+static struct twl4030_madc_bat_platform_data *
+	twl4030_madc_dt_probe(struct platform_device *pdev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif
+
 static int twl4030_madc_battery_probe(struct platform_device *pdev)
 {
 	struct twl4030_madc_battery *twl4030_madc_bat;
@@ -194,6 +271,9 @@ static int twl4030_madc_battery_probe(struct platform_device *pdev)
 	if (!twl4030_madc_bat)
 		return -ENOMEM;
 
+	if (!pdata)
+		pdata = twl4030_madc_dt_probe(pdev);
+
 	twl4030_madc_bat->psy.name = "twl4030_battery";
 	twl4030_madc_bat->psy.type = POWER_SUPPLY_TYPE_BATTERY;
 	twl4030_madc_bat->psy.properties = twl4030_madc_bat_props;
@@ -263,6 +343,7 @@ static int twl4030_madc_battery_remove(struct platform_device *pdev)
 static struct platform_driver twl4030_madc_battery_driver = {
 	.driver = {
 		.name = "twl4030_madc_battery",
+		.of_match_table = of_match_ptr(of_twl4030_madc_match),
 	},
 	.probe  = twl4030_madc_battery_probe,
 	.remove = twl4030_madc_battery_remove,
-- 
1.9.1

[PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Marek Belisko]

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt

diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
new file mode 100644
index 0000000..d3dd9d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
@@ -0,0 +1,43 @@
+twl4030_madc_battery
+
+Required properties:
+ - compatible : "ti,twl4030-madc-battery"
+ - capacity-uah : battery capacity in uAh
+ - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
+	for charging calibration (see example)
+ - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
+	for discharging calibration (see example)
+ - io-channels: Should contain IIO channel specifiers
+	for each element in io-channel-names.
+- io-channel-names: Should contain the following values:
+ * "temp" - The ADC channel for temperature reading
+ * "ichg" - The ADC channel for battery charging status
+ * "vbat" - The ADC channel to measure the battery voltage
+
+Example:
+	madc-battery {
+		compatible = "ti,twl4030-madc-battery";
+		capacity-uah = <1200000>;
+		ti,volt-to-capacity-charging-map = <4200 100>,
+					        <4100 75>,
+					        <4000 55>,
+					        <3900 25>,
+					        <3800 5>,
+					        <3700 2>,
+					        <3600 1>,
+					        <3300 0>;
+
+		ti,volt-to-capacity-discharging-map = <4200 100>
+						   <4100 95>,
+						   <4000 70>,
+						   <3800 50>,
+						   <3700 10>,
+						   <3600 5>,
+						   <3300 0>;
+		io-channels = <&twl_madc 1>,
+	                      <&twl_madc 10>,
+			      <&twl_madc 12>;
+		io-channel-names = "temp",
+		                   "ichg",
+		                   "vbat";
+	};
-- 
1.9.1

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Tony Lindgren]

Hi,

* Marek Belisko <marek@goldelico.com> [150310 14:28]:
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> 
> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> new file mode 100644
> index 0000000..d3dd9d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> @@ -0,0 +1,43 @@
> +twl4030_madc_battery
> +
> +Required properties:
> + - compatible : "ti,twl4030-madc-battery"
> + - capacity-uah : battery capacity in uAh
> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
> +	for charging calibration (see example)
> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
> +	for discharging calibration (see example)
> + - io-channels: Should contain IIO channel specifiers
> +	for each element in io-channel-names.
> +- io-channel-names: Should contain the following values:
> + * "temp" - The ADC channel for temperature reading
> + * "ichg" - The ADC channel for battery charging status
> + * "vbat" - The ADC channel to measure the battery voltage
> +
> +Example:
> +	madc-battery {
> +		compatible = "ti,twl4030-madc-battery";
> +		capacity-uah = <1200000>;
> +		ti,volt-to-capacity-charging-map = <4200 100>,
> +					        <4100 75>,
> +					        <4000 55>,
> +					        <3900 25>,
> +					        <3800 5>,
> +					        <3700 2>,
> +					        <3600 1>,
> +					        <3300 0>;
> +
> +		ti,volt-to-capacity-discharging-map = <4200 100>
> +						   <4100 95>,
> +						   <4000 70>,
> +						   <3800 50>,
> +						   <3700 10>,
> +						   <3600 5>,
> +						   <3300 0>;
> +		io-channels = <&twl_madc 1>,
> +	                      <&twl_madc 10>,
> +			      <&twl_madc 12>;
> +		io-channel-names = "temp",
> +		                   "ichg",
> +		                   "vbat";
> +	};

Rather than just making platform_data into device tree properties..

Can't you hide the these custom properties behind the compatible flag?

You can initialize that data in the driver based on the compatible
flag and the match data.

This makes sense if you can group things to similar configurations.

Regards,

Tony

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi,

Am 11.03.2015 um 16:24 schrieb Tony Lindgren <tony@atomide.com>:

> Hi,
> 
> * Marek Belisko <marek@goldelico.com> [150310 14:28]:
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>> .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> new file mode 100644
>> index 0000000..d3dd9d8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> @@ -0,0 +1,43 @@
>> +twl4030_madc_battery
>> +
>> +Required properties:
>> + - compatible : "ti,twl4030-madc-battery"
>> + - capacity-uah : battery capacity in uAh
>> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
>> +	for charging calibration (see example)
>> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
>> +	for discharging calibration (see example)
>> + - io-channels: Should contain IIO channel specifiers
>> +	for each element in io-channel-names.
>> +- io-channel-names: Should contain the following values:
>> + * "temp" - The ADC channel for temperature reading
>> + * "ichg" - The ADC channel for battery charging status
>> + * "vbat" - The ADC channel to measure the battery voltage
>> +
>> +Example:
>> +	madc-battery {
>> +		compatible = "ti,twl4030-madc-battery";
>> +		capacity-uah = <1200000>;
>> +		ti,volt-to-capacity-charging-map = <4200 100>,
>> +					        <4100 75>,
>> +					        <4000 55>,
>> +					        <3900 25>,
>> +					        <3800 5>,
>> +					        <3700 2>,
>> +					        <3600 1>,
>> +					        <3300 0>;
>> +
>> +		ti,volt-to-capacity-discharging-map = <4200 100>
>> +						   <4100 95>,
>> +						   <4000 70>,
>> +						   <3800 50>,
>> +						   <3700 10>,
>> +						   <3600 5>,
>> +						   <3300 0>;
>> +		io-channels = <&twl_madc 1>,
>> +	                      <&twl_madc 10>,
>> +			      <&twl_madc 12>;
>> +		io-channel-names = "temp",
>> +		                   "ichg",
>> +		                   "vbat";
>> +	};
> 
> Rather than just making platform_data into device tree properties..
> 
> Can't you hide the these custom properties behind the compatible flag?
> 
> You can initialize that data in the driver based on the compatible
> flag and the match data.
> 
> This makes sense if you can group things to similar configurations.

Maybe I have not completely understood your proposal.

Do you mean to go back to have big parameter tables for each device/battery
combination in the driver code and the compatible flag (e.g. compatible = “board17”)
chooses the right data set for the charging map and channels?

I thought this is what the DT was introduced for - to have the same driver 
code but adapt to different boards depending on hardware variations.

And batteries have very different characteristics and vary between devices…

The charging maps are depending on the battery type connected to the twl4030
and which madc channel is which value is also a little hardware dependent
(although the twl4030 doesn’t give much choice).

And moving this information into the driver for each board that uses it
would blow up the code.

BR,
Nikolaus

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Tony Lindgren]

* Dr. H. Nikolaus Schaller <hns@goldelico.com> [150311 09:17]:
> Hi,
> 
> Am 11.03.2015 um 16:24 schrieb Tony Lindgren <tony@atomide.com>:
> 
> > Hi,
> > 
> > * Marek Belisko <marek@goldelico.com> [150310 14:28]:
> >> Signed-off-by: Marek Belisko <marek@goldelico.com>
> >> ---
> >> .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++
> >> 1 file changed, 43 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> >> new file mode 100644
> >> index 0000000..d3dd9d8
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> >> @@ -0,0 +1,43 @@
> >> +twl4030_madc_battery
> >> +
> >> +Required properties:
> >> + - compatible : "ti,twl4030-madc-battery"
> >> + - capacity-uah : battery capacity in uAh
> >> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
> >> +	for charging calibration (see example)
> >> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
> >> +	for discharging calibration (see example)
> >> + - io-channels: Should contain IIO channel specifiers
> >> +	for each element in io-channel-names.
> >> +- io-channel-names: Should contain the following values:
> >> + * "temp" - The ADC channel for temperature reading
> >> + * "ichg" - The ADC channel for battery charging status
> >> + * "vbat" - The ADC channel to measure the battery voltage
> >> +
> >> +Example:
> >> +	madc-battery {
> >> +		compatible = "ti,twl4030-madc-battery";
> >> +		capacity-uah = <1200000>;
> >> +		ti,volt-to-capacity-charging-map = <4200 100>,
> >> +					        <4100 75>,
> >> +					        <4000 55>,
> >> +					        <3900 25>,
> >> +					        <3800 5>,
> >> +					        <3700 2>,
> >> +					        <3600 1>,
> >> +					        <3300 0>;
> >> +
> >> +		ti,volt-to-capacity-discharging-map = <4200 100>
> >> +						   <4100 95>,
> >> +						   <4000 70>,
> >> +						   <3800 50>,
> >> +						   <3700 10>,
> >> +						   <3600 5>,
> >> +						   <3300 0>;
> >> +		io-channels = <&twl_madc 1>,
> >> +	                      <&twl_madc 10>,
> >> +			      <&twl_madc 12>;
> >> +		io-channel-names = "temp",
> >> +		                   "ichg",
> >> +		                   "vbat";
> >> +	};
> > 
> > Rather than just making platform_data into device tree properties..
> > 
> > Can't you hide the these custom properties behind the compatible flag?
> > 
> > You can initialize that data in the driver based on the compatible
> > flag and the match data.
> > 
> > This makes sense if you can group things to similar configurations.
> 
> Maybe I have not completely understood your proposal.
> 
> Do you mean to go back to have big parameter tables for each device/battery
> combination in the driver code and the compatible flag (e.g. compatible = “board17”)
> chooses the right data set for the charging map and channels?

If you can somehow group them, then yes. Not for every board if there
are many of them naturally.
 
> I thought this is what the DT was introduced for - to have the same driver 
> code but adapt to different boards depending on hardware variations.

Yeah but you also need to consider the issues related to introducing
new device tree properties. The device tree properties introduced
should be generic where possible.

> And batteries have very different characteristics and vary between devices…

Right. Maybe that has been already agreed on to use capacity-uah for
batteries in general? In that case I have not problem with that as
it's a generic property :)
 
> The charging maps are depending on the battery type connected to the twl4030
> and which madc channel is which value is also a little hardware dependent
> (although the twl4030 doesn’t give much choice).

Just to consider alternatives before introducing driver specific
property for the maps.. Maybe here you could have few different type
of maps and select something safe by default? Of course it could be this
is higly board specific, I think some devices may be able to run below
3.3V for example..
 
> And moving this information into the driver for each board that uses it
> would blow up the code.

Right, I'm not saying we should build databases into the kernel drivers.
Just trying to avoid introducing driver specific properties unless
really needed :)

Regards,

Tony

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Am 11.03.2015 um 17:44 schrieb Tony Lindgren <tony@atomide.com>:

> * Dr. H. Nikolaus Schaller <hns@goldelico.com> [150311 09:17]:
>> Hi,
>> 
>> Am 11.03.2015 um 16:24 schrieb Tony Lindgren <tony@atomide.com>:
>> 
>>> Hi,
>>> 
>>> * Marek Belisko <marek@goldelico.com> [150310 14:28]:
>>>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>>>> ---
>>>> .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++
>>>> 1 file changed, 43 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>>> new file mode 100644
>>>> index 0000000..d3dd9d8
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>>> @@ -0,0 +1,43 @@
>>>> +twl4030_madc_battery
>>>> +
>>>> +Required properties:
>>>> + - compatible : "ti,twl4030-madc-battery"
>>>> + - capacity-uah : battery capacity in uAh
>>>> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
>>>> +	for charging calibration (see example)
>>>> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
>>>> +	for discharging calibration (see example)
>>>> + - io-channels: Should contain IIO channel specifiers
>>>> +	for each element in io-channel-names.
>>>> +- io-channel-names: Should contain the following values:
>>>> + * "temp" - The ADC channel for temperature reading
>>>> + * "ichg" - The ADC channel for battery charging status
>>>> + * "vbat" - The ADC channel to measure the battery voltage
>>>> +
>>>> +Example:
>>>> +	madc-battery {
>>>> +		compatible = "ti,twl4030-madc-battery";
>>>> +		capacity-uah = <1200000>;
>>>> +		ti,volt-to-capacity-charging-map = <4200 100>,
>>>> +					        <4100 75>,
>>>> +					        <4000 55>,
>>>> +					        <3900 25>,
>>>> +					        <3800 5>,
>>>> +					        <3700 2>,
>>>> +					        <3600 1>,
>>>> +					        <3300 0>;
>>>> +
>>>> +		ti,volt-to-capacity-discharging-map = <4200 100>
>>>> +						   <4100 95>,
>>>> +						   <4000 70>,
>>>> +						   <3800 50>,
>>>> +						   <3700 10>,
>>>> +						   <3600 5>,
>>>> +						   <3300 0>;
>>>> +		io-channels = <&twl_madc 1>,
>>>> +	                      <&twl_madc 10>,
>>>> +			      <&twl_madc 12>;
>>>> +		io-channel-names = "temp",
>>>> +		                   "ichg",
>>>> +		                   "vbat";
>>>> +	};
>>> 
>>> Rather than just making platform_data into device tree properties..
>>> 
>>> Can't you hide the these custom properties behind the compatible flag?
>>> 
>>> You can initialize that data in the driver based on the compatible
>>> flag and the match data.
>>> 
>>> This makes sense if you can group things to similar configurations.
>> 
>> Maybe I have not completely understood your proposal.
>> 
>> Do you mean to go back to have big parameter tables for each device/battery
>> combination in the driver code and the compatible flag (e.g. compatible = “board17”)
>> chooses the right data set for the charging map and channels?
> 
> If you can somehow group them, then yes.

I don’t see how to group them. Could you make a proposal?

> Not for every board if there
> are many of them naturally.
> 
>> I thought this is what the DT was introduced for - to have the same driver 
>> code but adapt to different boards depending on hardware variations.
> 
> Yeah but you also need to consider the issues related to introducing
> new device tree properties. The device tree properties introduced
> should be generic where possible.

Yes, that was discussed for a while for this driver’s bindings leading to v4.

Which ones do you think are not generic enough?

> 
>> And batteries have very different characteristics and vary between devices…
> 
> Right. Maybe that has been already agreed on to use capacity-uah for
> batteries in general?

Ah, do you mean with generic/not generic the distinction between a “ti,” prefix
and no prefix?

Well, I don’t know if there is such an agreement and I would have no argument
against calling it “ti,capacity-uah”.

> In that case I have not problem with that as
> it’s a generic property :)

Well, many batteries and systems have a fuel gauge chip (e.g. bq27000). This
chip “knows” the capacity. But therefore it is not needed to specify
it anywhere because it can be read out (usually in uAh).

This driver is to solve the issue that there is no such factory-programmed
battery or fuel gauge chip connected to a twl4030 driver. Nobody can program
that capacity value - except someone matching the device tree with real hardware.

And, by doing and averaging some charge-discharge cycles the fuel gauge
mapping is calibrated.

> 
>> The charging maps are depending on the battery type connected to the twl4030
>> and which madc channel is which value is also a little hardware dependent
>> (although the twl4030 doesn’t give much choice).
> 
> Just to consider alternatives before introducing driver specific
> property for the maps.. Maybe here you could have few different type
> of maps and select something safe by default? Of course it could be this
> is higly board specific, I think some devices may be able to run below
> 3.3V for example..

Every battery setup has a different map (which also might depend on the
series resistance of the battery and the wiring).

> 
>> And moving this information into the driver for each board that uses it
>> would blow up the code.
> 
> Right, I'm not saying we should build databases into the kernel drivers.
> Just trying to avoid introducing driver specific properties unless
> really needed :)

They are not really driver specific, they are battery specific. They
describe properties of the battery:

* capacity - depends on battery
* voltage depending on current charging level - depends on battery (and differs between charging and discharging)
* wiring of MADC inputs to the twl4030 is board dependent.

So all these properties are not driver specific, but describe hardware.
And therefore they had been introduced exactly this way into the old
platform_data driver.

So if you want to see a “ti," prefix to the capacity property I would be
fine.

BR,
Nikolaus

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Tony Lindgren]

* Dr. H. Nikolaus Schaller <hns@goldelico.com> [150311 10:13]:
> Am 11.03.2015 um 17:44 schrieb Tony Lindgren <tony@atomide.com>:
> > * Dr. H. Nikolaus Schaller <hns@goldelico.com> [150311 09:17]:
> >> Am 11.03.2015 um 16:24 schrieb Tony Lindgren <tony@atomide.com>:
> >>> 
> >>> Rather than just making platform_data into device tree properties..
> >>> 
> >>> Can't you hide the these custom properties behind the compatible flag?
> >>> 
> >>> You can initialize that data in the driver based on the compatible
> >>> flag and the match data.
> >>> 
> >>> This makes sense if you can group things to similar configurations.
> >> 
> >> Maybe I have not completely understood your proposal.
> >> 
> >> Do you mean to go back to have big parameter tables for each device/battery
> >> combination in the driver code and the compatible flag (e.g. compatible = “board17”)
> >> chooses the right data set for the charging map and channels?
> > 
> > If you can somehow group them, then yes.
> 
> I don’t see how to group them. Could you make a proposal?

Sorry no idea about that :) I though you may have some ideas based on
dealing with the driver.
 
> >> I thought this is what the DT was introduced for - to have the same driver 
> >> code but adapt to different boards depending on hardware variations.
> > 
> > Yeah but you also need to consider the issues related to introducing
> > new device tree properties. The device tree properties introduced
> > should be generic where possible.
> 
> Yes, that was discussed for a while for this driver’s bindings leading to v4.
> 
> Which ones do you think are not generic enough?

It seems maps is the only one then, assuming the cpacity-uah can be made
Linux generic. 

> >> And batteries have very different characteristics and vary between devices…
> > 
> > Right. Maybe that has been already agreed on to use capacity-uah for
> > batteries in general?
> 
> Ah, do you mean with generic/not generic the distinction between a “ti,” prefix
> and no prefix?
> 
> Well, I don’t know if there is such an agreement and I would have no argument
> against calling it “ti,capacity-uah”.

No no, "capacity-uah" is what we should use, but you need an ack from
the battery and device tree people that this is OK. Let's not add
"ti,capacity-uah” as that can obviously be a generic property.
 
> > In that case I have not problem with that as
> > it’s a generic property :)
> 
> Well, many batteries and systems have a fuel gauge chip (e.g. bq27000). This
> chip “knows” the capacity. But therefore it is not needed to specify
> it anywhere because it can be read out (usually in uAh).
> 
> This driver is to solve the issue that there is no such factory-programmed
> battery or fuel gauge chip connected to a twl4030 driver. Nobody can program
> that capacity value - except someone matching the device tree with real hardware.
> 
> And, by doing and averaging some charge-discharge cycles the fuel gauge
> mapping is calibrated.

OK 
 
> >> The charging maps are depending on the battery type connected to the twl4030
> >> and which madc channel is which value is also a little hardware dependent
> >> (although the twl4030 doesn’t give much choice).
> > 
> > Just to consider alternatives before introducing driver specific
> > property for the maps.. Maybe here you could have few different type
> > of maps and select something safe by default? Of course it could be this
> > is higly board specific, I think some devices may be able to run below
> > 3.3V for example..
> 
> Every battery setup has a different map (which also might depend on the
> series resistance of the battery and the wiring).
> 
> > 
> >> And moving this information into the driver for each board that uses it
> >> would blow up the code.
> > 
> > Right, I'm not saying we should build databases into the kernel drivers.
> > Just trying to avoid introducing driver specific properties unless
> > really needed :)
> 
> They are not really driver specific, they are battery specific. They
> describe properties of the battery:
> 
> * capacity - depends on battery
> * voltage depending on current charging level - depends on battery (and differs between charging and discharging)
> * wiring of MADC inputs to the twl4030 is board dependent.
> 
> So all these properties are not driver specific, but describe hardware.
> And therefore they had been introduced exactly this way into the old
> platform_data driver.
> 
> So if you want to see a “ti," prefix to the capacity property I would be
> fine.

Oh if they are battery spicific, then ideally we'd have generic batery
voltage to capacity maps property rather than a custom ti specific
property.

To avoid extra hassles later on, maybe you could submit a generic
binding patch only documenting it to the battery people and the device
tree people? That will make it easier to maintain this driver in the
long run.

Regards,

Tony

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Sebastian Reichel]

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

Hi,

On Wed, Mar 11, 2015 at 10:43:17AM -0700, Tony Lindgren wrote:
> No no, "capacity-uah" is what we should use, but you need an ack from
> the battery and device tree people that this is OK. Let's not add
> "ti,capacity-uah” as that can obviously be a generic property.

I'm okay with capacity-uah.

> > [...]
> 
> Oh if they are battery spicific, then ideally we'd have generic batery
> voltage to capacity maps property rather than a custom ti specific
> property.
> 
> To avoid extra hassles later on, maybe you could submit a generic
> binding patch only documenting it to the battery people and the device
> tree people? That will make it easier to maintain this driver in the
> long run.

Actually the proper way would be to differentiate between the
battery and the measurement chip / adc and that should be
implemented in the long run. The kernel's power supply framework
is not yet ready for it, though.

Example DT:

battery {
    battery-specific-data;
};

fuel-gauge {
    measures = <&battery>;
};

charger {
    charges = <&battery>;
};

Since infrastructure for generic bindings is missing, I think its
best to have the vendor properties for now and map this to generic
properties, once they have been specified.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Tony Lindgren]

* Sebastian Reichel <sre@kernel.org> [150311 12:37]:
> Hi,
> 
> On Wed, Mar 11, 2015 at 10:43:17AM -0700, Tony Lindgren wrote:
> > No no, "capacity-uah" is what we should use, but you need an ack from
> > the battery and device tree people that this is OK. Let's not add
> > "ti,capacity-uah” as that can obviously be a generic property.
> 
> I'm okay with capacity-uah.

OK great.
 
> > > [...]
> > 
> > Oh if they are battery spicific, then ideally we'd have generic batery
> > voltage to capacity maps property rather than a custom ti specific
> > property.
> > 
> > To avoid extra hassles later on, maybe you could submit a generic
> > binding patch only documenting it to the battery people and the device
> > tree people? That will make it easier to maintain this driver in the
> > long run.
> 
> Actually the proper way would be to differentiate between the
> battery and the measurement chip / adc and that should be
> implemented in the long run. The kernel's power supply framework
> is not yet ready for it, though.
> 
> Example DT:
> 
> battery {
>     battery-specific-data;
> };
> 
> fuel-gauge {
>     measures = <&battery>;
> };
> 
> charger {
>     charges = <&battery>;
> };
> 
> Since infrastructure for generic bindings is missing, I think its
> best to have the vendor properties for now and map this to generic
> properties, once they have been specified.

OK, sounds good to me. I'm fine with the $subject patch as it is then:

Acked-by: Tony Lindgren <tony@atomide.com>

Tony

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Pavel Machek]

Hi!

> > >> +		io-channels = <&twl_madc 1>,
> > >> +	                      <&twl_madc 10>,
> > >> +			      <&twl_madc 12>;
> > >> +		io-channel-names = "temp",
> > >> +		                   "ichg",
> > >> +		                   "vbat";
> > >> +	};
> > > 
> > > Rather than just making platform_data into device tree properties..
> > > 
> > > Can't you hide the these custom properties behind the compatible flag?
> > > 
> > > You can initialize that data in the driver based on the compatible
> > > flag and the match data.
> > > 
> > > This makes sense if you can group things to similar configurations.
> > 
> > Maybe I have not completely understood your proposal.
> > 
> > Do you mean to go back to have big parameter tables for each device/battery
> > combination in the driver code and the compatible flag (e.g. compatible = “board17”)
> > chooses the right data set for the charging map and channels?
> 
> If you can somehow group them, then yes. Not for every board if there
> are many of them naturally.
>  
> > I thought this is what the DT was introduced for - to have the same driver 
> > code but adapt to different boards depending on hardware variations.
> 
> Yeah but you also need to consider the issues related to introducing
> new device tree properties. The device tree properties introduced
> should be generic where possible.
> 
> > And batteries have very different characteristics and vary between devices…
> 
> Right. Maybe that has been already agreed on to use capacity-uah for
> batteries in general? In that case I have not problem with that as
> it's a generic property :)
>  
> > The charging maps are depending on the battery type connected to the twl4030
> > and which madc channel is which value is also a little hardware dependent
> > (although the twl4030 doesn’t give much choice).
> 
> Just to consider alternatives before introducing driver specific
> property for the maps.. Maybe here you could have few different type
> of maps and select something safe by default? Of course it could be this
> is higly board specific, I think some devices may be able to run below
> 3.3V for example..

As I explained in some other mail, those tables should not be
neccessary at all. They can be computed from li-ion characteristics
and internal resistance, and assumed current during charge and
discharge.

Running below 3.3V.. not really. At that point, the battery is really
_empty_, and voltage is going down really really fast.

Plus, you are damaging the battery at that point.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 31.03.2015 um 09:26 schrieb Pavel Machek <pavel@ucw.cz>:

> Hi!
> 
>>>>> +		io-channels = <&twl_madc 1>,
>>>>> +	                      <&twl_madc 10>,
>>>>> +			      <&twl_madc 12>;
>>>>> +		io-channel-names = "temp",
>>>>> +		                   "ichg",
>>>>> +		                   "vbat";
>>>>> +	};
>>>> 
>>>> Rather than just making platform_data into device tree properties..
>>>> 
>>>> Can't you hide the these custom properties behind the compatible flag?
>>>> 
>>>> You can initialize that data in the driver based on the compatible
>>>> flag and the match data.
>>>> 
>>>> This makes sense if you can group things to similar configurations.
>>> 
>>> Maybe I have not completely understood your proposal.
>>> 
>>> Do you mean to go back to have big parameter tables for each device/battery
>>> combination in the driver code and the compatible flag (e.g. compatible = “board17”)
>>> chooses the right data set for the charging map and channels?
>> 
>> If you can somehow group them, then yes. Not for every board if there
>> are many of them naturally.
>> 
>>> I thought this is what the DT was introduced for - to have the same driver 
>>> code but adapt to different boards depending on hardware variations.
>> 
>> Yeah but you also need to consider the issues related to introducing
>> new device tree properties. The device tree properties introduced
>> should be generic where possible.
>> 
>>> And batteries have very different characteristics and vary between devices…
>> 
>> Right. Maybe that has been already agreed on to use capacity-uah for
>> batteries in general? In that case I have not problem with that as
>> it's a generic property :)
>> 
>>> The charging maps are depending on the battery type connected to the twl4030
>>> and which madc channel is which value is also a little hardware dependent
>>> (although the twl4030 doesn’t give much choice).
>> 
>> Just to consider alternatives before introducing driver specific
>> property for the maps.. Maybe here you could have few different type
>> of maps and select something safe by default? Of course it could be this
>> is higly board specific, I think some devices may be able to run below
>> 3.3V for example..
> 
> As I explained in some other mail, those tables should not be
> neccessary at all. They can be computed from li-ion characteristics
> and internal resistance, and assumed current during charge and
> discharge.

I already explained that we do not know the charging and discharging
current well enough for such a calculation.

And I explained that the “internal resistance” is a system (battery + cables +
connectors + other circuits) parameter that is not easy to derive or measure
and type into the .dts source code.

At least I have no idea how I should find it out for my boards. While I can
easily determine the curves (and we already have them for the platform_data
driver).

Please propose your own code doing that so that we can test if it is
better.

> 
> Running below 3.3V.. not really. At that point, the battery is really
> _empty_, and voltage is going down really really fast.

It is the diffference between 2% and 0% where a fuel indication might
be most important…

> 
> Plus, you are damaging the battery at that point.

The power controller will shut down - but the driver should report
reasonable (but IMHO not necessarily perfect) values until the last
moment.

BR,
Nikolaus

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Pavel Machek]

Hi!

> > As I explained in some other mail, those tables should not be
> > neccessary at all. They can be computed from li-ion characteristics
> > and internal resistance, and assumed current during charge and
> > discharge.
> 
> I already explained that we do not know the charging and discharging
> current well enough for such a calculation.
> 
> And I explained that the “internal resistance” is a system (battery + cables +
> connectors + other circuits) parameter that is not easy to derive or measure
> and type into the .dts source code.
> 
> At least I have no idea how I should find it out for my boards. While I can
> easily determine the curves (and we already have them for the platform_data
> driver).
> 
> Please propose your own code doing that so that we can test if it is
> better.

So, how does this look?

It looks to me like you have cca 0.1 Ohm resistance in your system,
and are using cca 75mA while discharging, and charge by cca 1.4A. (And
these are all the coefficients the code should need; rest is battery
characteristics -- common to all li-ions, and charger characteristics
-- that will be common to all cellphones. If current can be measured,
this code should go more precise answers).

pavel@amd:~/g/tui/ofone$ ./liion_maps
Charging
Voltage  4.2 V ; table  100 %  internal voltage 4.18 V current 0.233 A computed  97 %
Voltage  4.1 V ; table  75 %  internal voltage 4.08 V current 0.233 A computed  87 %
Voltage  4.0 V ; table  55 %  internal voltage 3.93 V current 0.700 A computed  69 %
Voltage  3.9 V ; table  25 %  internal voltage 3.76 V current 1.400 A computed  26 %
Voltage  3.8 V ; table  5 %  internal voltage 3.66 V current 1.400 A computed  3 %
Voltage  3.7 V ; table  2 %  internal voltage 3.56 V current 1.400 A computed  2 %
Voltage  3.6 V ; table  1 %  internal voltage 3.46 V current 1.400 A computed  1 %
Voltage  3.3 V ; table  0 %  internal voltage 3.16 V current 1.400 A computed  -1 %
Badness  395.4861761427434
Discharging
Voltage  4.2 V ; table  100 %  internal voltage 4.21 V current -0.075 A computed  100 %
Voltage  4.1 V ; table  95 %  internal voltage 4.11 V current -0.075 A computed  91 %
Voltage  4.0 V ; table  70 %  internal voltage 4.01 V current -0.075 A computed  79 %
Voltage  3.8 V ; table  50 %  internal voltage 3.81 V current -0.075 A computed  46 %
Voltage  3.7 V ; table  10 %  internal voltage 3.71 V current -0.075 A computed  3 %
Voltage  3.6 V ; table  5 %  internal voltage 3.61 V current -0.075 A computed  2 %
Voltage  3.3 V ; table  0 %  internal voltage 3.31 V current -0.075 A computed  0 %
Badness  171.69576218433212

> > Running below 3.3V.. not really. At that point, the battery is really
> > _empty_, and voltage is going down really really fast.
> 
> It is the diffference between 2% and 0% where a fuel indication might
> be most important…

> > Plus, you are damaging the battery at that point.
> 
> The power controller will shut down - but the driver should report
> reasonable (but IMHO not necessarily perfect) values until the last
> moment.

It is tricky to do a good job near 0%... or anywhere else. See for
example

http://cseweb.ucsd.edu/~trosing/lectures/battery.pdf

You start a call, percentage goes down, end a call, it will go
back up. I'm pretty sure you will not be able to make a call with "5%"
indication from your code at low battery temperature (say -10C).

Anyway, see above, I think I provide reasonable values even in that range.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
									Pavel

#!/usr/bin/python3
import math

def percent_internal(v):
    u = 0.0387-(1.4523*(3.7835-v))
    if u < 0:
        # Formula above does gives 19.66% for 3.756, and refuses to
        # work below that. Assume 3.3V is empty battery, and provide
        # linear dependency below that.
        u = (v - 3.3) * ((3.756 - 3.3) * 19.66)
        return u
    return (0.1966+math.sqrt(u))*100

charging = [ [4200, 100], [4100, 75], [4000, 55], [3900, 25], [3800, 5], [3700, 2], [3600, 1], [3300, 0] ]

discharging = [ [4200, 100], [4100, 95], [4000, 70], [3800, 50], [3700, 10], [3600, 5], [3300, 0] ]

# current > 0: charging
def percent_ohm(v, current):
    v_int = v - current * 0.1
    print(" internal voltage %1.2f V current %.3f A " % (v_int, current), end='')
    return percent_internal(v_int)

def percent(v, charging):
    if charging:
        # Charger model. Chargers will do constant current then
        # constant voltage, so current will go down as voltage
        # approaches 4.2V
        i = 2.8
        if v >= 4.:
            i = i/2
        if v >= 4.05:
            i = i/3
    else:
        i = -0.15

    # With current forced to 0, we get badness 4014 and 258
    # 2.5A, sloped: badness 576
    # +4A -> badness 1293
    # +3A -> badness 890
    # +2.5A ->         339
    # +2.4A -> badness 389
    # +2.3A -> badness 444    
    # +2.2A -> badness 504
    # +2A -> badness 634
    # +1A -> badness 1450
    # +0.5A -> badness 2865
    # -0.2A -> badness 252
    # -0.15A -> badness 251.37
    # -0.1A -> badness 252
    # -0.05A -> badness 254
    i/=2
    return percent_ohm(v, i)

def compute(map, charging):
    diff = 0
    if charging:
        print("Charging")
    else:
        print("Discharging")
    for v, p in map:
        v /= 1000.
        print("Voltage ", v, "V ; table ", p, "% ", end='')
        p2 = percent(v, charging)
        print("computed ", int(p2), "%")
        diff += (p-p2)*(p-p2)
    print("Badness ", diff)

                
#perc = percent(volt)
compute(charging, 1)
compute(discharging, 0)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi,

Am 01.04.2015 um 22:16 schrieb Pavel Machek <pavel@ucw.cz>:

> Hi!
> 
>>> As I explained in some other mail, those tables should not be
>>> neccessary at all. They can be computed from li-ion characteristics
>>> and internal resistance, and assumed current during charge and
>>> discharge.
>> 
>> I already explained that we do not know the charging and discharging
>> current well enough for such a calculation.
>> 
>> And I explained that the “internal resistance” is a system (battery + cables +
>> connectors + other circuits) parameter that is not easy to derive or measure
>> and type into the .dts source code.
>> 
>> At least I have no idea how I should find it out for my boards. While I can
>> easily determine the curves (and we already have them for the platform_data
>> driver).
>> 
>> Please propose your own code doing that so that we can test if it is
>> better.
> 
> So, how does this look?
> 
> It looks to me like you have cca 0.1 Ohm resistance in your system,

This is completely unknown.

> and are using cca 75mA while discharging, and charge by cca 1.4A.

Where do you get these figures from?

A GTA04 board discharges usually between 50 and 400 mA (depending on activities)
and if you turn on WiFi, it will be up to 600 mA. If you use 3G it can draw even more
during operation.

Total current going in is 500-800 mA (depending on USB negotiations) and this is
split into system operation and charging current. So 1.4A charging current is not
possible. Rather 200-500 mA.

So it might be that the battery is discharged although the system is connected
to a charger.

> (And
> these are all the coefficients the code should need; rest is battery
> characteristics -- common to all li-ions, and charger characteristics
> -- that will be common to all cellphones. If current can be measured,
> this code should go more precise answers).
> 
> pavel@amd:~/g/tui/ofone$ ./liion_maps
> Charging
> Voltage  4.2 V ; table  100 %  internal voltage 4.18 V current 0.233 A computed  97 %
> Voltage  4.1 V ; table  75 %  internal voltage 4.08 V current 0.233 A computed  87 %
> Voltage  4.0 V ; table  55 %  internal voltage 3.93 V current 0.700 A computed  69 %
> Voltage  3.9 V ; table  25 %  internal voltage 3.76 V current 1.400 A computed  26 %
> Voltage  3.8 V ; table  5 %  internal voltage 3.66 V current 1.400 A computed  3 %
> Voltage  3.7 V ; table  2 %  internal voltage 3.56 V current 1.400 A computed  2 %
> Voltage  3.6 V ; table  1 %  internal voltage 3.46 V current 1.400 A computed  1 %
> Voltage  3.3 V ; table  0 %  internal voltage 3.16 V current 1.400 A computed  -1 %
> Badness  395.4861761427434
> Discharging
> Voltage  4.2 V ; table  100 %  internal voltage 4.21 V current -0.075 A computed  100 %
> Voltage  4.1 V ; table  95 %  internal voltage 4.11 V current -0.075 A computed  91 %
> Voltage  4.0 V ; table  70 %  internal voltage 4.01 V current -0.075 A computed  79 %
> Voltage  3.8 V ; table  50 %  internal voltage 3.81 V current -0.075 A computed  46 %
> Voltage  3.7 V ; table  10 %  internal voltage 3.71 V current -0.075 A computed  3 %
> Voltage  3.6 V ; table  5 %  internal voltage 3.61 V current -0.075 A computed  2 %
> Voltage  3.3 V ; table  0 %  internal voltage 3.31 V current -0.075 A computed  0 %
> Badness  171.69576218433212
> 
>>> Running below 3.3V.. not really. At that point, the battery is really
>>> _empty_, and voltage is going down really really fast.
>> 
>> It is the diffference between 2% and 0% where a fuel indication might
>> be most important…
> 
>>> Plus, you are damaging the battery at that point.
>> 
>> The power controller will shut down - but the driver should report
>> reasonable (but IMHO not necessarily perfect) values until the last
>> moment.
> 
> It is tricky to do a good job near 0%... or anywhere else. See for
> example
> 
> http://cseweb.ucsd.edu/~trosing/lectures/battery.pdf
> 
> You start a call, percentage goes down, end a call, it will go
> back up. I'm pretty sure you will not be able to make a call with "5%"
> indication from your code at low battery temperature (say -10C).

The whole system is heating up a little, so that you never have -10C for the
battery.

I think you are trying to solve situations that don’t exist in practice.

And AFAIK, the GTA04 board is the only user of this driver in case the battery
has no built-in fuel gauge. So why improve it beyond what the GTA04 users need?

I repeat myself: this driver is not built for highest precision because there are
better methods (fuel gauge chip). These might not be available so this fall-back
driver has been built.

It is definitively better than no driver and worse than the optimum.

> 
> Anyway, see above, I think I provide reasonable values even in that range.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 									Pavel
> 
> #!/usr/bin/python3
> import math
> 
> def percent_internal(v):
>    u = 0.0387-(1.4523*(3.7835-v))
>    if u < 0:
>        # Formula above does gives 19.66% for 3.756, and refuses to
>        # work below that. Assume 3.3V is empty battery, and provide
>        # linear dependency below that.
>        u = (v - 3.3) * ((3.756 - 3.3) * 19.66)
>        return u
>    return (0.1966+math.sqrt(u))*100
> 
> charging = [ [4200, 100], [4100, 75], [4000, 55], [3900, 25], [3800, 5], [3700, 2], [3600, 1], [3300, 0] ]
> 
> discharging = [ [4200, 100], [4100, 95], [4000, 70], [3800, 50], [3700, 10], [3600, 5], [3300, 0] ]
> 
> # current > 0: charging
> def percent_ohm(v, current):
>    v_int = v - current * 0.1
>    print(" internal voltage %1.2f V current %.3f A " % (v_int, current), end='')
>    return percent_internal(v_int)
> 
> def percent(v, charging):
>    if charging:
>        # Charger model. Chargers will do constant current then
>        # constant voltage, so current will go down as voltage
>        # approaches 4.2V
>        i = 2.8
>        if v >= 4.:
>            i = i/2
>        if v >= 4.05:
>            i = i/3
>    else:
>        i = -0.15
> 
>    # With current forced to 0, we get badness 4014 and 258
>    # 2.5A, sloped: badness 576
>    # +4A -> badness 1293
>    # +3A -> badness 890
>    # +2.5A ->         339
>    # +2.4A -> badness 389
>    # +2.3A -> badness 444    
>    # +2.2A -> badness 504
>    # +2A -> badness 634
>    # +1A -> badness 1450
>    # +0.5A -> badness 2865
>    # -0.2A -> badness 252
>    # -0.15A -> badness 251.37
>    # -0.1A -> badness 252
>    # -0.05A -> badness 254
>    i/=2
>    return percent_ohm(v, i)
> 
> def compute(map, charging):
>    diff = 0
>    if charging:
>        print("Charging")
>    else:
>        print("Discharging")
>    for v, p in map:
>        v /= 1000.
>        print("Voltage ", v, "V ; table ", p, "% ", end='')
>        p2 = percent(v, charging)
>        print("computed ", int(p2), "%")
>        diff += (p-p2)*(p-p2)
>    print("Badness ", diff)
> 
> 
> #perc = percent(volt)
> compute(charging, 1)
> compute(discharging, 0)

Please explain what you calculate here. Especially what “Badness” is?

BR,
Nikolaus

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Pavel Machek]

Hi!

> >> Please propose your own code doing that so that we can test if it is
> >> better.
> > 
> > So, how does this look?
> > 
> > It looks to me like you have cca 0.1 Ohm resistance in your system,
> 
> This is completely unknown.
> 
> > and are using cca 75mA while discharging, and charge by cca 1.4A.
> 
> Where do you get these figures from?

Least squares fitting of my coefficients to your tables.

> A GTA04 board discharges usually between 50 and 400 mA (depending on activities)
> and if you turn on WiFi, it will be up to 600 mA. If you use 3G it can draw even more
> during operation.

How big battery do you have? According to least squares fitting,
assuming maximum charge of .46A, you were taking about 25mA when
doing the discharge test.

> Total current going in is 500-800 mA (depending on USB negotiations) and this is
> split into system operation and charging current. So 1.4A charging current is not
> possible. Rather 200-500 mA.
> 
> So it might be that the battery is discharged although the system is connected
> to a charger.

Yah, and your battery meter will be wrong in such case, as will be
mine, because they are based on same information. The thing is, mine
can be improved without adding more tables. 

> > It is tricky to do a good job near 0%... or anywhere else. See for
> > example
> > 
> > http://cseweb.ucsd.edu/~trosing/lectures/battery.pdf
> > 
> > You start a call, percentage goes down, end a call, it will go
> > back up. I'm pretty sure you will not be able to make a call with "5%"
> > indication from your code at low battery temperature (say -10C).
> 
> The whole system is heating up a little, so that you never have -10C for the
> battery.

Umm. When not calling, your phone should not heat itself up. And you
definitely can power it down, leave it in outer pocket, then power it
up. It is actually something people do when they want to preserve battery...

> I think you are trying to solve situations that don’t exist in practice.
> 
> And AFAIK, the GTA04 board is the only user of this driver in case the battery
> has no built-in fuel gauge. So why improve it beyond what the GTA04
> users need?

Because then other users can share the same code, and because you
avoid big ugly tables in dts.

> I repeat myself: this driver is not built for highest precision because there are
> better methods (fuel gauge chip). These might not be available so this fall-back
> driver has been built.
> 
> It is definitively better than no driver and worse than the optimum.

And my email suggested solution better than your driver, so why not
just use it?


> > #perc = percent(volt)
> > compute(charging, 1)
> > compute(discharging, 0)
> 
> Please explain what you calculate here. Especially what “Badness” is?

Badness is error in least squares method.

Here are updated tables:

pavel@duo:~/g/tui/ofone$ ./liion_maps
Charging
Voltage  4.2 V ; table  100 %  internal voltage 4.18 V current 0.078 A computed  97 %
Voltage  4.1 V ; table  75 %  internal voltage 4.08 V current 0.078 A computed  87 %
Voltage  4.0 V ; table  55 %  internal voltage 3.93 V current 0.233 A computed  69 %
Voltage  3.9 V ; table  25 %  internal voltage 3.76 V current 0.467 A computed  26 %
Voltage  3.8 V ; table  5 %  internal voltage 3.66 V current 0.467 A computed  3 %
Voltage  3.7 V ; table  2 %  internal voltage 3.56 V current 0.467 A computed  2 %
Voltage  3.6 V ; table  1 %  internal voltage 3.46 V current 0.467 A computed  1 %
Voltage  3.3 V ; table  0 %  internal voltage 3.16 V current 0.467 A computed  -1 %
Badness  395.4861761427434
Discharging
Voltage  4.2 V ; table  100 %  internal voltage 4.21 V current -0.025 A computed  100 %
Voltage  4.1 V ; table  95 %  internal voltage 4.11 V current -0.025 A computed  91 %
Voltage  4.0 V ; table  70 %  internal voltage 4.01 V current -0.025 A computed  79 %
Voltage  3.8 V ; table  50 %  internal voltage 3.81 V current -0.025 A computed  46 %
Voltage  3.7 V ; table  10 %  internal voltage 3.71 V current -0.025 A computed  3 %
Voltage  3.6 V ; table  5 %  internal voltage 3.61 V current -0.025 A computed  2 %
Voltage  3.3 V ; table  0 %  internal voltage 3.31 V current -0.025 A computed  0 %
Badness  171.69576218433212
pavel@duo:~/g/tui/ofone$ python

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi Pavel,

Am 04.04.2015 um 10:16 schrieb Pavel Machek <pavel@ucw.cz>:

> Hi!
> 
>>>> Please propose your own code doing that so that we can test if it is
>>>> better.
>>> 
>>> So, how does this look?
>>> 
>>> It looks to me like you have cca 0.1 Ohm resistance in your system,
>> 
>> This is completely unknown.
>> 
>>> and are using cca 75mA while discharging, and charge by cca 1.4A.
>> 
>> Where do you get these figures from?
> 
> Least squares fitting of my coefficients to your tables.

Ah, I see.

> 
>> A GTA04 board discharges usually between 50 and 400 mA (depending on activities)
>> and if you turn on WiFi, it will be up to 600 mA. If you use 3G it can draw even more
>> during operation.
> 
> How big battery do you have?

1200 mAh

> According to least squares fitting,
> assuming maximum charge of .46A, you were taking about 25mA when
> doing the discharge test.

No, that can’t fit well. But I do not remember who has done this calibration in which situation
because it was done many months ago for the platform data version. We have just reformatted
the table for the DT.

> 
>> Total current going in is 500-800 mA (depending on USB negotiations) and this is
>> split into system operation and charging current. So 1.4A charging current is not
>> possible. Rather 200-500 mA.
>> 
>> So it might be that the battery is discharged although the system is connected
>> to a charger.
> 
> Yah, and your battery meter will be wrong in such case, as will be
> mine, because they are based on same information.

Well, it is not “mine”, it is the platform_data based driver already in kernel.org
since ca. 3.12.

We have just updated it to DT to get rid of platform_data in this area as well…

Yes, I see your argument that hiding the tables (two characteristic curves)
into an analytic approximation can be superior. 

The problem I see is just that your formula needs some parameters which
are difficult to derive or estimate. And must be adjusted to the specific
battery and system setup in a non-trivial way.

At least we must supply a tool (in the kernel/scripts directory?) where someone
can can estimate the parameters of the formula from the characteristic curves.

Maybe a tool that automatically runs several charge/discharge cycles at
different system load. And measures time vs. voltage. And assumes a linear
capacity decrease between the end points. (i.e. if it needs 10 hours to charge
from completely empty to full, we can assume 50% after 5,0 hours).

So my goal is to measure all characteristics of the battery - and no need to
study a (potentially non-existing) data sheet.

If you can provide that for all parameters of your algorithm I am fine with it.

> The thing is, mine
> can be improved without adding more tables. 

How can you improve your algorithm without tweaking or adding new parameters?

> 
>>> It is tricky to do a good job near 0%... or anywhere else. See for
>>> example
>>> 
>>> http://cseweb.ucsd.edu/~trosing/lectures/battery.pdf
>>> 
>>> You start a call, percentage goes down, end a call, it will go
>>> back up. I'm pretty sure you will not be able to make a call with "5%"
>>> indication from your code at low battery temperature (say -10C).
>> 
>> The whole system is heating up a little, so that you never have -10C for the
>> battery.
> 
> Umm. When not calling, your phone should not heat itself up.

Yes, in suspend it needs <20 mA which does not heat or course.
But steady operation at 20-400 mA does significantly rise OMAP
temperature beyond environment temperature.

> And you
> definitely can power it down, leave it in outer pocket, then power it
> up. It is actually something people do when they want to preserve battery...
> 
>> I think you are trying to solve situations that don’t exist in practice.
>> 
>> And AFAIK, the GTA04 board is the only user of this driver in case the battery
>> has no built-in fuel gauge. So why improve it beyond what the GTA04
>> users need?
> 
> Because then other users can share the same code, and because you

But you have ugly parameters in dts that nobody can easily see in the schematics
and that are full of assumptions.

>From a perspective of uncertainty analysis, estimation of the internal parameters
needs a higher precision than the calibration points.

> avoid big ugly tables in dts.

Well, the biggest tables I have seen in dts are keyboard matrix codes…

> 
>> I repeat myself: this driver is not built for highest precision because there are
>> better methods (fuel gauge chip). These might not be available so this fall-back
>> driver has been built.
>> 
>> It is definitively better than no driver and worse than the optimum.
> 
> And my email suggested solution better than your driver, so why not
> just use it?

I am not yet convinced that it is better.

It just moves the (unavoidable) limitations (measuring multiple calibration points)
just to a different area (measuring the hidden and not precisely known parameter
of the system).

> 
> 
>>> #perc = percent(volt)
>>> compute(charging, 1)
>>> compute(discharging, 0)
>> 
>> Please explain what you calculate here. Especially what “Badness” is?
> 
> Badness is error in least squares method.

Ok, I see. Thanks for clarification.

BR,
NIkolaus

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Rob Herring]

On Tue, Mar 10, 2015 at 4:27 PM, Marek Belisko <marek@goldelico.com> wrote:
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
>  .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>
> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> new file mode 100644
> index 0000000..d3dd9d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
> @@ -0,0 +1,43 @@
> +twl4030_madc_battery
> +
> +Required properties:
> + - compatible : "ti,twl4030-madc-battery"

Is this a sub-node of the twl4030 or something? Please define where
this fits (hint: I would expect to be a sub node of a charging
controller or battery monitor).

> + - capacity-uah : battery capacity in uAh
> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
> +       for charging calibration (see example)
> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
> +       for discharging calibration (see example)

These seem like properties of the battery independent of the
battery/charging controller which is the twl4030. Ideally we would
define battery nodes generically and independent of the charge
controllers. Then there are smart batteries to consider too.

Rob

> + - io-channels: Should contain IIO channel specifiers
> +       for each element in io-channel-names.
> +- io-channel-names: Should contain the following values:
> + * "temp" - The ADC channel for temperature reading
> + * "ichg" - The ADC channel for battery charging status
> + * "vbat" - The ADC channel to measure the battery voltage
> +
> +Example:
> +       madc-battery {
> +               compatible = "ti,twl4030-madc-battery";
> +               capacity-uah = <1200000>;
> +               ti,volt-to-capacity-charging-map = <4200 100>,
> +                                               <4100 75>,
> +                                               <4000 55>,
> +                                               <3900 25>,
> +                                               <3800 5>,
> +                                               <3700 2>,
> +                                               <3600 1>,
> +                                               <3300 0>;
> +
> +               ti,volt-to-capacity-discharging-map = <4200 100>
> +                                                  <4100 95>,
> +                                                  <4000 70>,
> +                                                  <3800 50>,
> +                                                  <3700 10>,
> +                                                  <3600 5>,
> +                                                  <3300 0>;
> +               io-channels = <&twl_madc 1>,
> +                             <&twl_madc 10>,
> +                             <&twl_madc 12>;
> +               io-channel-names = "temp",
> +                                  "ichg",
> +                                  "vbat";
> +       };
> --
> 1.9.1
>

Re: [PATCH v4 3/6] Documentation: DT: Document twl4030-madc-battery bindings

[Dr. H. Nikolaus Schaller]

Hi,

Am 01.04.2015 um 18:30 schrieb Rob Herring <robherring2@gmail.com>:

> On Tue, Mar 10, 2015 at 4:27 PM, Marek Belisko <marek@goldelico.com> wrote:
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>> .../bindings/power_supply/twl4030_madc_battery.txt | 43 ++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> new file mode 100644
>> index 0000000..d3dd9d8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> @@ -0,0 +1,43 @@
>> +twl4030_madc_battery
>> +
>> +Required properties:
>> + - compatible : "ti,twl4030-madc-battery"
> 
> Is this a sub-node of the twl4030 or something? Please define where
> this fits (hint: I would expect to be a sub node of a charging
> controller or battery monitor).

It is a driver connecting some battery parameters with measurements provided
by the twl4030-madc to present a /sys/class/power_supply node for the battery
with a coarse estimate for the charging level.

So maybe the best wording is that it is a battery-driver assuming a twl4030-madc
providing raw measurements (voltage, charging).

Therefore it could as well be battery-twl4030-madc.

> 
>> + - capacity-uah : battery capacity in uAh
>> + - ti,volt-to-capacity-charging-map : list of voltage(mV):level(%) values
>> +       for charging calibration (see example)
>> + - ti,volt-to-capacity-discharging-map : list of voltage(mV):level(%) values
>> +       for discharging calibration (see example)
> 
> These seem like properties of the battery independent of the
> battery/charging controller which is the twl4030. Ideally we would
> define battery nodes generically and independent of the charge
> controllers. Then there are smart batteries to consider too.

For smart batteries there are completely independent mechanisms
like I2C and HDQ/1-wire communication with fuel gauge chips (e.g. b27xxx).

Those do not need such battery properties because they are stored
in EEPROMs within these chips.

So all this is a quite special solution just for those handful of board that
have no smart battery and use exactly this twl4030 chip.

It is not intended to become a generic charging/fuel solution. Just
make it work with DT (it worked with platform_data since ~3.12).

BR,
Nikolaus



> 
> Rob
> 
>> + - io-channels: Should contain IIO channel specifiers
>> +       for each element in io-channel-names.
>> +- io-channel-names: Should contain the following values:
>> + * "temp" - The ADC channel for temperature reading
>> + * "ichg" - The ADC channel for battery charging status
>> + * "vbat" - The ADC channel to measure the battery voltage
>> +
>> +Example:
>> +       madc-battery {
>> +               compatible = "ti,twl4030-madc-battery";
>> +               capacity-uah = <1200000>;
>> +               ti,volt-to-capacity-charging-map = <4200 100>,
>> +                                               <4100 75>,
>> +                                               <4000 55>,
>> +                                               <3900 25>,
>> +                                               <3800 5>,
>> +                                               <3700 2>,
>> +                                               <3600 1>,
>> +                                               <3300 0>;
>> +
>> +               ti,volt-to-capacity-discharging-map = <4200 100>
>> +                                                  <4100 95>,
>> +                                                  <4000 70>,
>> +                                                  <3800 50>,
>> +                                                  <3700 10>,
>> +                                                  <3600 5>,
>> +                                                  <3300 0>;
>> +               io-channels = <&twl_madc 1>,
>> +                             <&twl_madc 10>,
>> +                             <&twl_madc 12>;
>> +               io-channel-names = "temp",
>> +                                  "ichg",
>> +                                  "vbat";
>> +       };
>> --
>> 1.9.1
>> 

[PATCH v4 4/6] ARM: dts: omap3-gta04: Add battery support

[Marek Belisko]

Added battery support for gta04 devices.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index fb3a696..cbf515a 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -49,6 +49,32 @@
 		ti,codec = <&twl_audio>;
 	};
 
+	battery {
+		compatible = "ti,twl4030-madc-battery";
+		capacity-uah = <1200000>;
+		ti,volt-to-capacity-charging-map = <4200 100>,
+					     <4100 75>,
+					     <4000 55>,
+					     <3900 25>,
+					     <3800 5>,
+					     <3700 2>,
+					     <3600 1>,
+					     <3300 0>;
+		ti,volt-to-capacity-discharging-map = <4200 100>,
+						<4100 95>,
+						<4000 70>,
+						<3800 50>,
+						<3700 10>,
+						<3600 5>,
+						<3300 0>;
+		io-channels = <&twl_madc 1>,
+	                      <&twl_madc 10>,
+			      <&twl_madc 12>;
+		io-channel-names = "temp",
+		                   "ichg",
+		                   "vbat";
+	};
+
 	spi_lcd {
 		compatible = "spi-gpio";
 		#address-cells = <0x1>;
@@ -518,3 +544,7 @@
 &mcbsp2 {
 	status = "okay";
 };
+
+&twl_madc {
+	ti,system-uses-second-madc-irq;
+};
-- 
1.9.1

Re: [PATCH v4 4/6] ARM: dts: omap3-gta04: Add battery support

[Tony Lindgren]

* Marek Belisko <marek@goldelico.com> [150310 14:28]:
> Added battery support for gta04 devices.
> 
> Signed-off-by: Marek Belisko <marek@goldelico.com>

Picking up this patch into omap-for-v4.1/dt branch thanks.

Tony

> ---
>  arch/arm/boot/dts/omap3-gta04.dtsi | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index fb3a696..cbf515a 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -49,6 +49,32 @@
>  		ti,codec = <&twl_audio>;
>  	};
>  
> +	battery {
> +		compatible = "ti,twl4030-madc-battery";
> +		capacity-uah = <1200000>;
> +		ti,volt-to-capacity-charging-map = <4200 100>,
> +					     <4100 75>,
> +					     <4000 55>,
> +					     <3900 25>,
> +					     <3800 5>,
> +					     <3700 2>,
> +					     <3600 1>,
> +					     <3300 0>;
> +		ti,volt-to-capacity-discharging-map = <4200 100>,
> +						<4100 95>,
> +						<4000 70>,
> +						<3800 50>,
> +						<3700 10>,
> +						<3600 5>,
> +						<3300 0>;
> +		io-channels = <&twl_madc 1>,
> +	                      <&twl_madc 10>,
> +			      <&twl_madc 12>;
> +		io-channel-names = "temp",
> +		                   "ichg",
> +		                   "vbat";
> +	};
> +
>  	spi_lcd {
>  		compatible = "spi-gpio";
>  		#address-cells = <0x1>;
> @@ -518,3 +544,7 @@
>  &mcbsp2 {
>  	status = "okay";
>  };
> +
> +&twl_madc {
> +	ti,system-uses-second-madc-irq;
> +};
> -- 
> 1.9.1
> 

Re: [PATCH v4 4/6] ARM: dts: omap3-gta04: Add battery support

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [150316 13:59]:
> * Marek Belisko <marek@goldelico.com> [150310 14:28]:
> > Added battery support for gta04 devices.
> > 
> > Signed-off-by: Marek Belisko <marek@goldelico.com>
> 
> Picking up this patch into omap-for-v4.1/dt branch thanks.

Actually not yet applying this as looks like Pavel just
had few comments on these properties.
 
> Tony
> 
> > ---
> >  arch/arm/boot/dts/omap3-gta04.dtsi | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> > index fb3a696..cbf515a 100644
> > --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> > +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> > @@ -49,6 +49,32 @@
> >  		ti,codec = <&twl_audio>;
> >  	};
> >  
> > +	battery {
> > +		compatible = "ti,twl4030-madc-battery";
> > +		capacity-uah = <1200000>;
> > +		ti,volt-to-capacity-charging-map = <4200 100>,
> > +					     <4100 75>,
> > +					     <4000 55>,
> > +					     <3900 25>,
> > +					     <3800 5>,
> > +					     <3700 2>,
> > +					     <3600 1>,
> > +					     <3300 0>;
> > +		ti,volt-to-capacity-discharging-map = <4200 100>,
> > +						<4100 95>,
> > +						<4000 70>,
> > +						<3800 50>,
> > +						<3700 10>,
> > +						<3600 5>,
> > +						<3300 0>;
> > +		io-channels = <&twl_madc 1>,
> > +	                      <&twl_madc 10>,
> > +			      <&twl_madc 12>;
> > +		io-channel-names = "temp",
> > +		                   "ichg",
> > +		                   "vbat";
> > +	};
> > +
> >  	spi_lcd {
> >  		compatible = "spi-gpio";
> >  		#address-cells = <0x1>;
> > @@ -518,3 +544,7 @@
> >  &mcbsp2 {
> >  	status = "okay";
> >  };
> > +
> > +&twl_madc {
> > +	ti,system-uses-second-madc-irq;
> > +};
> > -- 
> > 1.9.1
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 5/6] power: twl4030_madc_battery: Add of_twl4030_madc_match to MODULE_DEVICE_TABLE

[Marek Belisko]

When twl_4030_madc_battery is build as module, MODULE_DEVICE_TABLE allow
the module to be auto-loaded since the module will match the devices
instantiated from device tree.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 drivers/power/twl4030_madc_battery.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
index 4bcb4a9..3e0005d 100644
--- a/drivers/power/twl4030_madc_battery.c
+++ b/drivers/power/twl4030_madc_battery.c
@@ -252,6 +252,8 @@ static const struct of_device_id of_twl4030_madc_match[] = {
 	{},
 };
 
+MODULE_DEVICE_TABLE(of, of_twl4030_madc_match);
+
 #else
 static struct twl4030_madc_bat_platform_data *
 	twl4030_madc_dt_probe(struct platform_device *pdev)
-- 
1.9.1

[PATCH v4 6/6] power: twl4030_madc_battery: Add missing MODULE_ALIAS

[Marek Belisko]

Without MODULE_ALIAS twl4030_madc_battery won't get loaded automatically.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 drivers/power/twl4030_madc_battery.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
index 3e0005d..d0753f1 100644
--- a/drivers/power/twl4030_madc_battery.c
+++ b/drivers/power/twl4030_madc_battery.c
@@ -355,3 +355,4 @@ module_platform_driver(twl4030_madc_battery_driver);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Lukas Märdian <lukas@goldelico.com>");
 MODULE_DESCRIPTION("twl4030_madc battery driver");
+MODULE_ALIAS("platform:twl4030_madc_battery");
-- 
1.9.1

Re: [PATCH v4 6/6] power: twl4030_madc_battery: Add missing MODULE_ALIAS

[Sebastian Reichel]

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

Hi,

On Tue, Mar 10, 2015 at 10:27:27PM +0100, Marek Belisko wrote:
> Without MODULE_ALIAS twl4030_madc_battery won't get loaded
> automatically.

Thanks, pulled.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]