[PATCH v2 0/3] Convert twl4030_madc_battery to IIO consumer and add DT aupport

[PATCH v2 0/3] Convert twl4030_madc_battery to IIO consumer and add DT aupport

[Marek Belisko]

This patches are based on Sebastian Reichel work [1] which convert twl4030_madc mfd to iio
framework. 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.

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

Marek Belisko (3):
  power: twl4030-madc-battery: Convert to iio consumer.
  power: twl4030_madc_battery: Add device tree support.
  Documentation: DT: Document twl4030-madc-battery bindings.

 .../bindings/power_supply/twl4030_madc_battery.txt |  43 ++++++
 drivers/power/twl4030_madc_battery.c               | 155 ++++++++++++++++-----
 2 files changed, 165 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt

-- 
1.8.3.2

[PATCH v2] 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>
---
 drivers/power/twl4030_madc_battery.c | 102 ++++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 36 deletions(-)

diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
index 7ef445a..0a64105 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,35 @@ 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;
-
-	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];
+	int val, err;
+	err = iio_read_channel_processed(channel, &val);
+	if (err < 0) {
+		pr_info("Error:%d\n", err);
+		return err;
+	}
+	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 +80,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 +115,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 +139,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 +188,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 +204,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 +232,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 +253,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.8.3.2

[PATCH v2] 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 0a64105..b446dd8 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;
@@ -184,6 +185,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", &pdata->capacity);
+	if (ret != 0)
+		return ERR_PTR(-EINVAL);
+
+	/* parse and prepare charging data */
+	proplen = of_property_count_u32_elems(np, "charging-calibration-data");
+	if (proplen < 0) {
+		dev_warn(&pdev->dev, "No 'charging-calibration-data' 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, "charging-calibration-data",
+					   i * 2,
+					   (u32 *)&pdata->charging[i].voltage);
+		of_property_read_u32_index(np, "charging-calibration-data",
+					  i * 2 + 1,
+					  (u32 *)&pdata->charging[i].level);
+	}
+
+	/* parse and prepare discharging data */
+	proplen = of_property_count_u32_elems(np,
+			"discharging-calibration-data");
+	if (proplen < 0) {
+		dev_warn(&pdev->dev, "No 'discharging-calibration-data' 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, "discharging-calibration-data",
+					   i * 2,
+					   (u32 *)&pdata->discharging[i].voltage);
+		of_property_read_u32_index(np, "discharging-calibration-data",
+					   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;
@@ -195,6 +272,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;
@@ -264,6 +344,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.8.3.2

[PATCH v2] 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..fd0b6d2
--- /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 : battery capacity in uAh
+ - charging-calibration-data : list of voltage(mV):level(%) values
+	for charging calibration (see example)
+ - discharging-calibration-data : 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 = <1200000>;
+		charging-calibration-data = <4200 100
+					     4100 75
+					     4000 55
+					     3900 25
+					     3800 5
+					     3700 2
+					     3600 1
+					     3300 0>;
+
+		discharging-calibration-data = <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.8.3.2

[PATCH v2] Documentation: DT: Document twl4030-madc-battery bindings.

[Marek Belisko]

Signed-off-by: Marek Belisko <marek-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
---
 .../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..fd0b6d2
--- /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 : battery capacity in uAh
+ - charging-calibration-data : list of voltage(mV):level(%) values
+	for charging calibration (see example)
+ - discharging-calibration-data : 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 = <1200000>;
+		charging-calibration-data = <4200 100
+					     4100 75
+					     4000 55
+					     3900 25
+					     3800 5
+					     3700 2
+					     3600 1
+					     3300 0>;
+
+		discharging-calibration-data = <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.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] power: twl4030-madc-battery: Convert to iio consumer.

[Sebastian Reichel]

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

Hi Marek,

I just have one comment:

On Wed, Mar 05, 2014 at 09:52:17PM +0100, Marek Belisko wrote:
> [...]
> -static int madc_read(int index)
> +static int madc_read(struct iio_channel *channel)
>  {
> -	struct twl4030_madc_request req;
> -	int val;
> -
> -	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];
> +	int val, err;
> +	err = iio_read_channel_processed(channel, &val);
> +	if (err < 0) {
> +		pr_info("Error:%d\n", err);

This should be "Error: %d\n" (with a space).

Apart from that you should use dev_info() or dev_err(), so that the
message is properly prefixed by the device. Currently there is no
hint where this error message is generated.

> +		return err;
> +	}
> +	return val;
>  }
> [...]

After fixing this you can add

Reviewed-By: Sebastian Reichel <sre@debian.org>

-- Sebastian

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

Re: [PATCH v2] power: twl4030-madc-battery: Convert to iio consumer.

[Belisko Marek]

Hi Sebastian,

On Thu, Mar 6, 2014 at 12:25 AM, Sebastian Reichel <sre@debian.org> wrote:
> Hi Marek,
>
> I just have one comment:
>
> On Wed, Mar 05, 2014 at 09:52:17PM +0100, Marek Belisko wrote:
>> [...]
>> -static int madc_read(int index)
>> +static int madc_read(struct iio_channel *channel)
>>  {
>> -     struct twl4030_madc_request req;
>> -     int val;
>> -
>> -     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];
>> +     int val, err;
>> +     err = iio_read_channel_processed(channel, &val);
>> +     if (err < 0) {
>> +             pr_info("Error:%d\n", err);
>
> This should be "Error: %d\n" (with a space).
Ups this is remain from debugging messages. I'll remove it and re-post patches.
Thanks.
>
> Apart from that you should use dev_info() or dev_err(), so that the
> message is properly prefixed by the device. Currently there is no
> hint where this error message is generated.
>
>> +             return err;
>> +     }
>> +     return val;
>>  }
>> [...]
>
> After fixing this you can add
>
> Reviewed-By: Sebastian Reichel <sre@debian.org>
>
> -- Sebastian

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH v2] power: twl4030-madc-battery: Convert to iio consumer.

[Belisko Marek]

Hi Sebastian,

can you please take this series (I'll post update version with
removing debug code). Thanks.

On Thu, Mar 6, 2014 at 10:10 PM, Belisko Marek <marek.belisko@gmail.com> wrote:
> Hi Sebastian,
>
> On Thu, Mar 6, 2014 at 12:25 AM, Sebastian Reichel <sre@debian.org> wrote:
>> Hi Marek,
>>
>> I just have one comment:
>>
>> On Wed, Mar 05, 2014 at 09:52:17PM +0100, Marek Belisko wrote:
>>> [...]
>>> -static int madc_read(int index)
>>> +static int madc_read(struct iio_channel *channel)
>>>  {
>>> -     struct twl4030_madc_request req;
>>> -     int val;
>>> -
>>> -     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];
>>> +     int val, err;
>>> +     err = iio_read_channel_processed(channel, &val);
>>> +     if (err < 0) {
>>> +             pr_info("Error:%d\n", err);
>>
>> This should be "Error: %d\n" (with a space).
> Ups this is remain from debugging messages. I'll remove it and re-post patches.
> Thanks.
>>
>> Apart from that you should use dev_info() or dev_err(), so that the
>> message is properly prefixed by the device. Currently there is no
>> hint where this error message is generated.
>>
>>> +             return err;
>>> +     }
>>> +     return val;
>>>  }
>>> [...]
>>
>> After fixing this you can add
>>
>> Reviewed-By: Sebastian Reichel <sre@debian.org>
>>
>> -- Sebastian
>
> BR,
>
> marek
>
> --
> as simple and primitive as possible
> -------------------------------------------------
> Marek Belisko - OPEN-NANDRA
> Freelance Developer
>
> Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
> Tel: +421 915 052 184
> skype: marekwhite
> twitter: #opennandra
> web: http://open-nandra.com

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH v2] power: twl4030-madc-battery: Convert to iio consumer.

[Sebastian Reichel]

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

Hi Marek,

On Mon, Aug 11, 2014 at 09:52:52PM +0200, Belisko Marek wrote:
> can you please take this series (I'll post update version with
> removing debug code). Thanks.

mh. I will not pull this with "(dis)charging-calibration-data" as
DT property name without an ACK from the DT binding maintainers.

I would feel fine with pulling this when they are prefixed with
"ti,". Otherwise the series looks good to me.

-- Sebastian

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

Re: [PATCH v2] power: twl4030-madc-battery: Convert to iio consumer.

[Belisko Marek]

Hi Sebastian,

On Thu, Aug 14, 2014 at 11:03 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi Marek,
>
> On Mon, Aug 11, 2014 at 09:52:52PM +0200, Belisko Marek wrote:
>> can you please take this series (I'll post update version with
>> removing debug code). Thanks.
>
> mh. I will not pull this with "(dis)charging-calibration-data" as
> DT property name without an ACK from the DT binding maintainers.
DT maintainers ping.
>
> I would feel fine with pulling this when they are prefixed with
> "ti,". Otherwise the series looks good to me.
Why it should be prefixed by "ti"? Isn't enough to have "ti" prefix for driver
compatible property? Thanks.
>
> -- Sebastian

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH v2] power: twl4030-madc-battery: Convert to iio consumer.

[Mark Rutland]

On Thu, Sep 18, 2014 at 09:13:47PM +0100, Belisko Marek wrote:
> Hi Sebastian,
> 
> On Thu, Aug 14, 2014 at 11:03 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi Marek,
> >
> > On Mon, Aug 11, 2014 at 09:52:52PM +0200, Belisko Marek wrote:
> >> can you please take this series (I'll post update version with
> >> removing debug code). Thanks.
> >
> > mh. I will not pull this with "(dis)charging-calibration-data" as
> > DT property name without an ACK from the DT binding maintainers.
> DT maintainers ping.

Apologies for the delay. I will take a look at the bidning shortly.

> >
> > I would feel fine with pulling this when they are prefixed with
> > "ti,". Otherwise the series looks good to me.
> Why it should be prefixed by "ti"? Isn't enough to have "ti" prefix for driver
> compatible property? Thanks.

Because if we want to add a similar generic property, the names will
clash.

Ideally bindings should use prefixed properties by default -- those can
be replaced with generic properties as commonality becomes apparent.

Mark.

Re: [PATCH v2] Documentation: DT: Document twl4030-madc-battery bindings.

[Mark Rutland]

Hi,

On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko 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..fd0b6d2
> --- /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"

I'm rather confused by this; the code seems to have stripped away all
the twl4030 dependencies and this now seems to be a generic ADC battery
status driver. So why does this need to be specific to twl4030?

> + - capacity : battery capacity in uAh

I would make the units explicit in the name; "capacity" can be measured
in many ways.

> + - charging-calibration-data : list of voltage(mV):level(%) values
> +	for charging calibration (see example)
> + - discharging-calibration-data : list of voltage(mV):level(%) values
> +	for discharging calibration (see example)

Regardless of the prefixing of these property names, the specifics are
unclear to me.

Does this apply to vbat?

Is vbat guaranteed to read in mV?

Is any particular order required? The example seems to go be in
descending percentage values.

Is any particular interpolation expected on behalf of the OS, or is this
just an informative set of measurements that hte OS can use as it sees
fit?

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

Which units? Where is this used?

> + * "ichg" - The ADC channel for battery charging status

How exactly is this used to detect the charging status?

> + * "vbat" - The ADC channel to measure the battery voltage
> +
> +Example:
> +	madc-battery {
> +		compatible = "ti,twl4030-madc-battery";
> +		capacity = <1200000>;
> +		charging-calibration-data = <4200 100
> +					     4100 75
> +					     4000 55
> +					     3900 25
> +					     3800 5
> +					     3700 2
> +					     3600 1
> +					     3300 0>;

Please bracket list entries individually (as you've done with
io-channels).

Thanks,
Mark.

> +
> +		discharging-calibration-data = <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.8.3.2
> 
> 

Re: [PATCH v2] Documentation: DT: Document twl4030-madc-battery bindings.

[Belisko Marek]

Hi Mark,

On Mon, Sep 22, 2014 at 1:03 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko 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..fd0b6d2
>> --- /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"
>
> I'm rather confused by this; the code seems to have stripped away all
> the twl4030 dependencies and this now seems to be a generic ADC battery
> status driver. So why does this need to be specific to twl4030?
You are right. Also possibly driver name is incorrect :). Better name could be
ti, madc-battery.
>
>> + - capacity : battery capacity in uAh
>
> I would make the units explicit in the name; "capacity" can be measured
> in many ways.
OK capacity_uAh should be fine I believe.
>
>> + - charging-calibration-data : list of voltage(mV):level(%) values
>> +     for charging calibration (see example)
>> + - discharging-calibration-data : list of voltage(mV):level(%) values
>> +     for discharging calibration (see example)
>
> Regardless of the prefixing of these property names, the specifics are
> unclear to me.
Do you suggest to rename it completely or is enough to add "ti, " prefix?
>
> Does this apply to vbat?
Yes bat is read and then interpolation algorithm is applied for
various power properties.
So mapping from voltage to percentage is defined in table.
>
> Is vbat guaranteed to read in mV?
Yes.
>
> Is any particular order required? The example seems to go be in
> descending percentage values.
There is done sorting in driver so ordering it's not necessary.
>
> Is any particular interpolation expected on behalf of the OS, or is this
> just an informative set of measurements that hte OS can use as it sees
> fit?
>
>> + - io-channels: Should contain IIO channel specifiers
>> +     for each element in io-channel-names.
>> +- io-channel-names: Should contain the following values:
All 3 statements was copied from rx51-battery and documentation is the same.
There is not unit for all channels just raw values which are then
converted in driver.
>> + * "temp" - The ADC channel for temperature reading
>
> Which units? Where is this used?
>
>> + * "ichg" - The ADC channel for battery charging status
>
> How exactly is this used to detect the charging status?
>
>> + * "vbat" - The ADC channel to measure the battery voltage
>> +
>> +Example:
>> +     madc-battery {
>> +             compatible = "ti,twl4030-madc-battery";
>> +             capacity = <1200000>;
>> +             charging-calibration-data = <4200 100
>> +                                          4100 75
>> +                                          4000 55
>> +                                          3900 25
>> +                                          3800 5
>> +                                          3700 2
>> +                                          3600 1
>> +                                          3300 0>;
>
> Please bracket list entries individually (as you've done with
> io-channels).
OK.
>
> Thanks,
> Mark.
>
>> +
>> +             discharging-calibration-data = <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.8.3.2
>>
>>

Thanks,

Marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH v2] Documentation: DT: Document twl4030-madc-battery bindings.

[Mark Rutland]

On Mon, Sep 22, 2014 at 09:35:03PM +0100, Belisko Marek wrote:
> Hi Mark,
> 
> On Mon, Sep 22, 2014 at 1:03 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi,
> >
> > On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko 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..fd0b6d2
> >> --- /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"
> >
> > I'm rather confused by this; the code seems to have stripped away all
> > the twl4030 dependencies and this now seems to be a generic ADC battery
> > status driver. So why does this need to be specific to twl4030?
> You are right. Also possibly driver name is incorrect :). Better name could be
> ti, madc-battery.

Ok.

> >
> >> + - capacity : battery capacity in uAh
> >
> > I would make the units explicit in the name; "capacity" can be measured
> > in many ways.
> OK capacity_uAh should be fine I believe.

Well, "capacity-uah" to match naming conventions.

> >
> >> + - charging-calibration-data : list of voltage(mV):level(%) values
> >> +     for charging calibration (see example)
> >> + - discharging-calibration-data : list of voltage(mV):level(%) values
> >> +     for discharging calibration (see example)
> >
> > Regardless of the prefixing of these property names, the specifics are
> > unclear to me.
> Do you suggest to rename it completely or is enough to add "ti, " prefix?

I'm not suggesting prefixing. I'm suggesting that "calibration data"
could mean a variety of things, so having a more specific name would be
beneficial.

> > Does this apply to vbat?
> Yes bat is read and then interpolation algorithm is applied for
> various power properties.
> So mapping from voltage to percentage is defined in table.
> >
> > Is vbat guaranteed to read in mV?
> Yes.
> >
> > Is any particular order required? The example seems to go be in
> > descending percentage values.
> There is done sorting in driver so ordering it's not necessary.
> >
> > Is any particular interpolation expected on behalf of the OS, or is this
> > just an informative set of measurements that hte OS can use as it sees
> > fit?
> >
> >> + - io-channels: Should contain IIO channel specifiers
> >> +     for each element in io-channel-names.
> >> +- io-channel-names: Should contain the following values:
> All 3 statements was copied from rx51-battery and documentation is the same.
> There is not unit for all channels just raw values which are then
> converted in driver.

I don't understand. How do you figure out the mapping of those raw
values to sensible units? Surely you need additional information to do
so?

Mark.

Re: [PATCH v2] Documentation: DT: Document twl4030-madc-battery bindings.

[Belisko Marek]

Hi Mark,

On Fri, Sep 26, 2014 at 3:39 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Sep 22, 2014 at 09:35:03PM +0100, Belisko Marek wrote:
>> Hi Mark,
>>
>> On Mon, Sep 22, 2014 at 1:03 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Hi,
>> >
>> > On Wed, Mar 05, 2014 at 08:52:19PM +0000, Marek Belisko 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..fd0b6d2
>> >> --- /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"
>> >
>> > I'm rather confused by this; the code seems to have stripped away all
>> > the twl4030 dependencies and this now seems to be a generic ADC battery
>> > status driver. So why does this need to be specific to twl4030?
>> You are right. Also possibly driver name is incorrect :). Better name could be
>> ti, madc-battery.
>
> Ok.
>
>> >
>> >> + - capacity : battery capacity in uAh
>> >
>> > I would make the units explicit in the name; "capacity" can be measured
>> > in many ways.
>> OK capacity_uAh should be fine I believe.
>
> Well, "capacity-uah" to match naming conventions.
OK.
>
>> >
>> >> + - charging-calibration-data : list of voltage(mV):level(%) values
>> >> +     for charging calibration (see example)
>> >> + - discharging-calibration-data : list of voltage(mV):level(%) values
>> >> +     for discharging calibration (see example)
>> >
>> > Regardless of the prefixing of these property names, the specifics are
>> > unclear to me.
>> Do you suggest to rename it completely or is enough to add "ti, " prefix?
>
> I'm not suggesting prefixing. I'm suggesting that "calibration data"
> could mean a variety of things, so having a more specific name would be
> beneficial.
Charging and discharging data are used for some kind of interpolation
within voltage interval.
So we get volume from ADC and then convert it according
charging/discharging table to some value
which is then useded in various battery properties (status,
charge_now, capacity, time_to_empty_now).
So maybe battery-charging/discharging-calibration-data could be property name?

>
>> > Does this apply to vbat?
>> Yes bat is read and then interpolation algorithm is applied for
>> various power properties.
>> So mapping from voltage to percentage is defined in table.
>> >
>> > Is vbat guaranteed to read in mV?
>> Yes.
>> >
>> > Is any particular order required? The example seems to go be in
>> > descending percentage values.
>> There is done sorting in driver so ordering it's not necessary.
>> >
>> > Is any particular interpolation expected on behalf of the OS, or is this
>> > just an informative set of measurements that hte OS can use as it sees
>> > fit?
>> >
>> >> + - io-channels: Should contain IIO channel specifiers
>> >> +     for each element in io-channel-names.
>> >> +- io-channel-names: Should contain the following values:
>> All 3 statements was copied from rx51-battery and documentation is the same.
>> There is not unit for all channels just raw values which are then
>> converted in driver.
>
> I don't understand. How do you figure out the mapping of those raw
> values to sensible units? Surely you need additional information to do
> so?
Yes mapping from raw values is done in driver. Like current read from
iio channel_ichg
is multiplied by 1000 or read from channel_temp is multiplied by 10 to
have proper values for
battery properties.
>
> Mark.

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com