[PATCH v5 1/7] Add support for XO 1.75 to OLPC battery driver

[PATCH v5 1/7] Add support for XO 1.75 to OLPC battery driver

[Lubomir Rintel]

Hello!

This patch set modifies the OLPC battery driver so that it could eventually
be used on an Arm-based OLPC XO 1.75 machine once the Embedded Controlled
driver gets merged.

It has been split off from the EC driver patch set after four re-spins.

Tested on an OLPC XO 1. I don't have a XO 1.5, but it's similar enough.

[1/7] dt-bindings: olpc_battery: Add XO-1.5 battery
[2/7] x86, olpc: Use a correct version when making up a battery node
[3/7] power: supply: olpc_battery: Use DT to get battery version
[4/7] power: supply: olpc_battery: Move priv data to a struct
[5/7] power: supply: olpc_battery: Use devm_power_supply_register()
[6/7] power: supply: olpc_battery: Avoid using platform_info
[7/7] power: supply: olpc_battery: Add OLPC XO 1.75 support

Appart from the dt-bindings patch, the patches need to be applied in order.

Lubo

[PATCH v5 1/7] dt-bindings: olpc_battery: Add XO-1.5 battery

[Lubomir Rintel]

The XO-1 and XO-1.5 batteries apparently differ in an ability to report
ambient temperature.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

---
Changes since v1:
- Collected Reviewed-by and Acked-by tags

 Documentation/devicetree/bindings/power/supply/olpc_battery.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
index c8901b3992d9..8d87d6b35a98 100644
--- a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
@@ -2,4 +2,4 @@ OLPC battery
 ~~~~~~~~~~~~
 
 Required properties:
-  - compatible : "olpc,xo1-battery"
+  - compatible : "olpc,xo1-battery" or "olpc,xo1.5-battery"
-- 
2.20.1

[PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node

[Lubomir Rintel]

The XO-1 and XO-1.5 batteries apparently differ in an ability to report
ambient temperature. Add a different compatible string to the 1.5
battery.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>

---
Changes since v1:
- Avoid splitting string literals

 arch/x86/platform/olpc/olpc_dt.c | 84 +++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 28 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index b4ab779f1d47..8557add82752 100644
--- a/arch/x86/platform/olpc/olpc_dt.c
+++ b/arch/x86/platform/olpc/olpc_dt.c
@@ -217,10 +217,28 @@ static u32 __init olpc_dt_get_board_revision(void)
 	return be32_to_cpu(rev);
 }
 
-void __init olpc_dt_fixup(void)
+int olpc_dt_compatible_match(phandle node, const char *compat)
 {
-	int r;
 	char buf[64];
+	int plen;
+	char *p;
+	int len;
+
+	plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
+	if (plen <= 0)
+		return 0;
+
+	len = strlen(compat);
+	for (p = buf; p < buf + plen; p += strlen(p) + 1) {
+		if (strcmp(p, compat) == 0)
+			return 1;
+	}
+
+	return 0;
+}
+
+void __init olpc_dt_fixup(void)
+{
 	phandle node;
 	u32 board_rev;
 
@@ -228,41 +246,51 @@ void __init olpc_dt_fixup(void)
 	if (!node)
 		return;
 
-	/*
-	 * If the battery node has a compatible property, we are running a new
-	 * enough firmware and don't have fixups to make.
-	 */
-	r = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
-	if (r > 0)
-		return;
-
-	pr_info("PROM DT: Old firmware detected, applying fixes\n");
-
-	/* Add olpc,xo1-battery compatible marker to battery node */
-	olpc_dt_interpret("\" /battery@0\" find-device"
-		" \" olpc,xo1-battery\" +compatible"
-		" device-end");
-
 	board_rev = olpc_dt_get_board_revision();
 	if (!board_rev)
 		return;
 
 	if (board_rev >= olpc_board_pre(0xd0)) {
+		if (olpc_dt_compatible_match(node, "olpc,xo1.5-battery"))
+			return;
+
+		/* Add olpc,xo1.5-battery compatible marker to battery node */
+		olpc_dt_interpret("\" /battery@0\" find-device");
+		olpc_dt_interpret("  \" olpc,xo1.5-battery\" +compatible");
+		olpc_dt_interpret("device-end");
+
+		/* We're running a very old firmware if this is missing. */
+		if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
+			return;
+
 		/* XO-1.5: add dcon device */
-		olpc_dt_interpret("\" /pci/display@1\" find-device"
-			" new-device"
-			" \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
-			" finish-device device-end");
+		olpc_dt_interpret("\" /pci/display@1\" find-device");
+		olpc_dt_interpret("  new-device");
+		olpc_dt_interpret("    \" dcon\" device-name");
+		olpc_dt_interpret("    \" olpc,xo1-dcon\" +compatible");
+		olpc_dt_interpret("  finish-device");
+		olpc_dt_interpret("device-end");
 	} else {
+		/* We're running a very old firmware if this is missing. */
+		if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
+			return;
+
 		/* XO-1: add dcon device, mark RTC as olpc,xo1-rtc */
-		olpc_dt_interpret("\" /pci/display@1,1\" find-device"
-			" new-device"
-			" \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
-			" finish-device device-end"
-			" \" /rtc\" find-device"
-			" \" olpc,xo1-rtc\" +compatible"
-			" device-end");
+		olpc_dt_interpret("\" /pci/display@1,1\" find-device");
+		olpc_dt_interpret("  new-device");
+		olpc_dt_interpret("    \" dcon\" device-name");
+		olpc_dt_interpret("    \" olpc,xo1-dcon\" +compatible");
+		olpc_dt_interpret("  finish-device");
+		olpc_dt_interpret("device-end");
+		olpc_dt_interpret("\" /rtc\" find-device");
+		olpc_dt_interpret("  \" olpc,xo1-rtc\" +compatible");
+		olpc_dt_interpret("device-end");
 	}
+
+	/* Add olpc,xo1-battery compatible marker to battery node */
+	olpc_dt_interpret("\" /battery@0\" find-device");
+	olpc_dt_interpret("  \" olpc,xo1-battery\" +compatible");
+	olpc_dt_interpret("device-end");
 }
 
 void __init olpc_dt_build_devicetree(void)
-- 
2.20.1

[PATCH v5 3/7] power: supply: olpc_battery: Use DT to get battery version

[Lubomir Rintel]

Avoid using the x86 OLPC platform specific call to get the board
version. That wouldn't work on FDT-based ARM MMP2 platform.

Add the XO 1.5 compatible string too. This is actually not completely
necessary as the battery nodes on XO 1.5 claim to be compatible with
"olpc,xo1-battery", but there are, in fact, differencies.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

---
Changes since v2:
- Clarify the XO 1 compatibility in the commit message.

Changes since v1:
- Sort the new include a bit higher

 drivers/power/supply/olpc_battery.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 5a97e42a3547..5323987d9284 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -14,6 +14,7 @@
 #include <linux/types.h>
 #include <linux/err.h>
 #include <linux/device.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/jiffies.h>
@@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
 	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
 	if (IS_ERR(olpc_ac))
 		return PTR_ERR(olpc_ac);
-
-	if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
+	if (of_property_match_string(pdev->dev.of_node, "compatible",
+					"olpc,xo1.5-battery") >= 0) {
+		/* XO-1.5 */
 		olpc_bat_desc.properties = olpc_xo15_bat_props;
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
-	} else { /* XO-1 */
+	} else {
+		/* XO-1 */
 		olpc_bat_desc.properties = olpc_xo1_bat_props;
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
@@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device *pdev)
 
 static const struct of_device_id olpc_battery_ids[] = {
 	{ .compatible = "olpc,xo1-battery" },
+	{ .compatible = "olpc,xo1.5-battery" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, olpc_battery_ids);
-- 
2.20.1

[PATCH v5 4/7] power: supply: olpc_battery: Move priv data to a struct

[Lubomir Rintel]

The global variables for private data are not too nice. I'd like some
more, and that would clutter the global name space even further.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

---
Changes since v4:
- Bring some more code that was misplaced here.

Changes since v2:
- Bring the allocaton of the priv data structure here

Changes since v1:
- Split out the move to devm_* into a separate patch

 drivers/power/supply/olpc_battery.c | 78 ++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 30 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 5323987d9284..7067cb500669 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -53,6 +53,12 @@
 
 #define BAT_ADDR_MFR_TYPE	0x5F
 
+struct olpc_battery_data {
+	struct power_supply *olpc_ac;
+	struct power_supply *olpc_bat;
+	char bat_serial[17];
+};
+
 /*********************************************************************
  *		Power
  *********************************************************************/
@@ -91,11 +97,8 @@ static const struct power_supply_desc olpc_ac_desc = {
 	.get_property = olpc_ac_get_prop,
 };
 
-static struct power_supply *olpc_ac;
-
-static char bat_serial[17]; /* Ick */
-
-static int olpc_bat_get_status(union power_supply_propval *val, uint8_t ec_byte)
+static int olpc_bat_get_status(struct olpc_battery_data *data,
+		union power_supply_propval *val, uint8_t ec_byte)
 {
 	if (olpc_platform_info.ecver > 0x44) {
 		if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
@@ -326,6 +329,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 				 enum power_supply_property psp,
 				 union power_supply_propval *val)
 {
+	struct olpc_battery_data *data = power_supply_get_drvdata(psy);
 	int ret = 0;
 	__be16 ec_word;
 	uint8_t ec_byte;
@@ -347,7 +351,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
-		ret = olpc_bat_get_status(val, ec_byte);
+		ret = olpc_bat_get_status(data, val, ec_byte);
 		if (ret)
 			return ret;
 		break;
@@ -450,8 +454,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		sprintf(bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
-		val->strval = bat_serial;
+		sprintf(data->bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
+		val->strval = data->bat_serial;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
 		ret = olpc_bat_get_voltage_max_design(val);
@@ -579,17 +583,17 @@ static struct power_supply_desc olpc_bat_desc = {
 	.use_for_apm = 1,
 };
 
-static struct power_supply *olpc_bat;
-
 static int olpc_battery_suspend(struct platform_device *pdev,
 				pm_message_t state)
 {
-	if (device_may_wakeup(&olpc_ac->dev))
+	struct olpc_battery_data *data = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&data->olpc_ac->dev))
 		olpc_ec_wakeup_set(EC_SCI_SRC_ACPWR);
 	else
 		olpc_ec_wakeup_clear(EC_SCI_SRC_ACPWR);
 
-	if (device_may_wakeup(&olpc_bat->dev))
+	if (device_may_wakeup(&data->olpc_bat->dev))
 		olpc_ec_wakeup_set(EC_SCI_SRC_BATTERY | EC_SCI_SRC_BATSOC
 				   | EC_SCI_SRC_BATERR);
 	else
@@ -601,8 +605,15 @@ static int olpc_battery_suspend(struct platform_device *pdev,
 
 static int olpc_battery_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct power_supply_config psy_cfg = {};
+	struct olpc_battery_data *data;
 	uint8_t status;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, data);
 
 	/*
 	 * We've seen a number of EC protocol changes; this driver requires
@@ -620,9 +631,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
 
 	/* Ignore the status. It doesn't actually matter */
 
-	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
-	if (IS_ERR(olpc_ac))
-		return PTR_ERR(olpc_ac);
+	psy_cfg.of_node = pdev->dev.of_node;
+	psy_cfg.drv_data = data;
+
+	data->olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
+	if (IS_ERR(data->olpc_ac))
+		return PTR_ERR(data->olpc_ac);
+
 	if (of_property_match_string(pdev->dev.of_node, "compatible",
 					"olpc,xo1.5-battery") >= 0) {
 		/* XO-1.5 */
@@ -634,42 +649,45 @@ static int olpc_battery_probe(struct platform_device *pdev)
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
 
-	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
-	if (IS_ERR(olpc_bat)) {
-		ret = PTR_ERR(olpc_bat);
+	data->olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
+	if (IS_ERR(data->olpc_bat)) {
+		ret = PTR_ERR(data->olpc_bat);
 		goto battery_failed;
 	}
 
-	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
+	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	if (ret)
 		goto eeprom_failed;
 
-	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
+	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
 	if (ret)
 		goto error_failed;
 
 	if (olpc_ec_wakeup_available()) {
-		device_set_wakeup_capable(&olpc_ac->dev, true);
-		device_set_wakeup_capable(&olpc_bat->dev, true);
+		device_set_wakeup_capable(&data->olpc_ac->dev, true);
+		device_set_wakeup_capable(&data->olpc_bat->dev, true);
 	}
 
 	return 0;
 
 error_failed:
-	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
+	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 eeprom_failed:
-	power_supply_unregister(olpc_bat);
+	power_supply_unregister(data->olpc_bat);
 battery_failed:
-	power_supply_unregister(olpc_ac);
+	power_supply_unregister(data->olpc_ac);
 	return ret;
 }
 
 static int olpc_battery_remove(struct platform_device *pdev)
 {
-	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
-	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
-	power_supply_unregister(olpc_bat);
-	power_supply_unregister(olpc_ac);
+	struct olpc_battery_data *data = platform_get_drvdata(pdev);
+
+	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
+	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
+	power_supply_unregister(data->olpc_bat);
+	power_supply_unregister(data->olpc_ac);
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH v5 5/7] power: supply: olpc_battery: Use devm_power_supply_register()

[Lubomir Rintel]

This simplifies the error handling.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

---
Changes since v1:
- This was split off the "power: supply: olpc_battery: Move priv data to
  a struct" patch.

 drivers/power/supply/olpc_battery.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 7067cb500669..1fcc459433a8 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -634,7 +634,7 @@ static int olpc_battery_probe(struct platform_device *pdev)
 	psy_cfg.of_node = pdev->dev.of_node;
 	psy_cfg.drv_data = data;
 
-	data->olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
+	data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
 	if (IS_ERR(data->olpc_ac))
 		return PTR_ERR(data->olpc_ac);
 
@@ -649,15 +649,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
 
-	data->olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
-	if (IS_ERR(data->olpc_bat)) {
-		ret = PTR_ERR(data->olpc_bat);
-		goto battery_failed;
-	}
+	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
+	if (IS_ERR(data->olpc_bat))
+		return PTR_ERR(data->olpc_bat);
 
 	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	if (ret)
-		goto eeprom_failed;
+		return ret;
 
 	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
 	if (ret)
@@ -672,10 +670,6 @@ static int olpc_battery_probe(struct platform_device *pdev)
 
 error_failed:
 	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
-eeprom_failed:
-	power_supply_unregister(data->olpc_bat);
-battery_failed:
-	power_supply_unregister(data->olpc_ac);
 	return ret;
 }
 
@@ -685,9 +679,6 @@ static int olpc_battery_remove(struct platform_device *pdev)
 
 	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
 	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
-	power_supply_unregister(data->olpc_bat);
-	power_supply_unregister(data->olpc_ac);
-
 	return 0;
 }
 
-- 
2.20.1

[PATCH v5 6/7] power: supply: olpc_battery: Avoid using platform_info

[Lubomir Rintel]

This wouldn't work on the DT-based ARM platform. Let's read the EC version
directly from the EC driver instead.

This removes x86 specific bits that would prevent this driver from being
used with the EC of ARM-based OLPC XO 1.75.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

---
Changes since v2:
- Move the priv data allocation hunk from this patch to a proper place

Changes since v1:
- Use uint8_t instead of unsigned char [1] for ecver

 drivers/power/supply/olpc_battery.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 1fcc459433a8..a6c89d002d5d 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -22,7 +22,6 @@
 #include <linux/olpc-ec.h>
 #include <asm/olpc.h>
 
-
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
 #define EC_BAT_ACR	0x12	/* int16_t,	*6250/15,    µAh  */
@@ -57,6 +56,7 @@ struct olpc_battery_data {
 	struct power_supply *olpc_ac;
 	struct power_supply *olpc_bat;
 	char bat_serial[17];
+	int new_proto;
 };
 
 /*********************************************************************
@@ -100,7 +100,7 @@ static const struct power_supply_desc olpc_ac_desc = {
 static int olpc_bat_get_status(struct olpc_battery_data *data,
 		union power_supply_propval *val, uint8_t ec_byte)
 {
-	if (olpc_platform_info.ecver > 0x44) {
+	if (data->new_proto) {
 		if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
 		else if (ec_byte & BAT_STAT_DISCHARGING)
@@ -608,6 +608,7 @@ static int olpc_battery_probe(struct platform_device *pdev)
 	struct power_supply_config psy_cfg = {};
 	struct olpc_battery_data *data;
 	uint8_t status;
+	uint8_t ecver;
 	int ret;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
@@ -615,13 +616,21 @@ static int olpc_battery_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, data);
 
-	/*
-	 * We've seen a number of EC protocol changes; this driver requires
-	 * the latest EC protocol, supported by 0x44 and above.
-	 */
-	if (olpc_platform_info.ecver < 0x44) {
+	/* See if the EC is already there and get the EC revision */
+	ret = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0, &ecver, 1);
+	if (ret)
+		return ret;
+
+	if (ecver > 0x44) {
+		/* XO 1 or 1.5 with a new EC firmware. */
+		data->new_proto = 1;
+	} else if (ecver < 0x44) {
+		/*
+		 * We've seen a number of EC protocol changes; this driver
+		 * requires the latest EC protocol, supported by 0x44 and above.
+		 */
 		printk(KERN_NOTICE "OLPC EC version 0x%02x too old for "
-			"battery driver.\n", olpc_platform_info.ecver);
+			"battery driver.\n", ecver);
 		return -ENXIO;
 	}
 
-- 
2.20.1

[PATCH v5 7/7] power: supply: olpc_battery: Add OLPC XO 1.75 support

[Lubomir Rintel]

The battery and the protocol are essentially the same as OLPC XO 1.5,
but the responses from the EC are LSB first.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>

---
Changes since v2:
- Fix the version conditional

Changes since v1:
- s/s16 ecword_to_cpu/u16 ecword_to_cpu/
- s/u16 ec_byte/u16 ec_word/

 drivers/power/supply/olpc_battery.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index a6c89d002d5d..52de90049980 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -57,6 +57,7 @@ struct olpc_battery_data {
 	struct power_supply *olpc_bat;
 	char bat_serial[17];
 	int new_proto;
+	int little_endian;
 };
 
 /*********************************************************************
@@ -322,6 +323,14 @@ static int olpc_bat_get_voltage_max_design(union power_supply_propval *val)
 	return ret;
 }
 
+static u16 ecword_to_cpu(struct olpc_battery_data *data, u16 ec_word)
+{
+	if (data->little_endian)
+		return le16_to_cpu(ec_word);
+	else
+		return be16_to_cpu(ec_word);
+}
+
 /*********************************************************************
  *		Battery properties
  *********************************************************************/
@@ -394,7 +403,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 9760L / 32;
+		val->intval = ecword_to_cpu(data, ec_word) * 9760L / 32;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
@@ -402,7 +411,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 15625L / 120;
+		val->intval = ecword_to_cpu(data, ec_word) * 15625L / 120;
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
 		ret = olpc_ec_cmd(EC_BAT_SOC, NULL, 0, &ec_byte, 1);
@@ -433,21 +442,21 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
+		val->intval = ecword_to_cpu(data, ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
 		ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
 		if (ret)
 			return ret;
 
-		val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
+		val->intval = (int)ecword_to_cpu(data, ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
 		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 6250 / 15;
+		val->intval = ecword_to_cpu(data, ec_word) * 6250 / 15;
 		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
@@ -621,7 +630,11 @@ static int olpc_battery_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (ecver > 0x44) {
+	if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
+		/* XO 1.75 */
+		data->new_proto = 1;
+		data->little_endian = 1;
+	} else if (ecver > 0x44) {
 		/* XO 1 or 1.5 with a new EC firmware. */
 		data->new_proto = 1;
 	} else if (ecver < 0x44) {
-- 
2.20.1

Re: [PATCH v5 3/7] power: supply: olpc_battery: Use DT to get battery version

[Sebastian Reichel]

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

Hi,

On Thu, Jan 10, 2019 at 06:40:01PM +0100, Lubomir Rintel wrote:
> Avoid using the x86 OLPC platform specific call to get the board
> version. That wouldn't work on FDT-based ARM MMP2 platform.
> 
> Add the XO 1.5 compatible string too. This is actually not completely
> necessary as the battery nodes on XO 1.5 claim to be compatible with
> "olpc,xo1-battery", but there are, in fact, differencies.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> ---
> Changes since v2:
> - Clarify the XO 1 compatibility in the commit message.
> 
> Changes since v1:
> - Sort the new include a bit higher
> 
>  drivers/power/supply/olpc_battery.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..5323987d9284 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -14,6 +14,7 @@
>  #include <linux/types.h>
>  #include <linux/err.h>
>  #include <linux/device.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/jiffies.h>
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
>  	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
>  	if (IS_ERR(olpc_ac))
>  		return PTR_ERR(olpc_ac);
> -
> -	if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> +	if (of_property_match_string(pdev->dev.of_node, "compatible",
> +					"olpc,xo1.5-battery") >= 0) {

of_device_is_compatible(...)

-- Sebastian

> +		/* XO-1.5 */
>  		olpc_bat_desc.properties = olpc_xo15_bat_props;
>  		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
> -	} else { /* XO-1 */
> +	} else {
> +		/* XO-1 */
>  		olpc_bat_desc.properties = olpc_xo1_bat_props;
>  		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
>  	}
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id olpc_battery_ids[] = {
>  	{ .compatible = "olpc,xo1-battery" },
> +	{ .compatible = "olpc,xo1.5-battery" },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> -- 
> 2.20.1
> 

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

Re: [PATCH v5 6/7] power: supply: olpc_battery: Avoid using platform_info

[Sebastian Reichel]

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

Hi,

On Thu, Jan 10, 2019 at 06:40:04PM +0100, Lubomir Rintel wrote:
> This wouldn't work on the DT-based ARM platform. Let's read the EC version
> directly from the EC driver instead.
> 
> This removes x86 specific bits that would prevent this driver from being
> used with the EC of ARM-based OLPC XO 1.75.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> ---
> Changes since v2:
> - Move the priv data allocation hunk from this patch to a proper place
> 
> Changes since v1:
> - Use uint8_t instead of unsigned char [1] for ecver
> 
>  drivers/power/supply/olpc_battery.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 1fcc459433a8..a6c89d002d5d 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -22,7 +22,6 @@
>  #include <linux/olpc-ec.h>
>  #include <asm/olpc.h>
>  
> -
>  #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
>  #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
>  #define EC_BAT_ACR	0x12	/* int16_t,	*6250/15,    µAh  */
> @@ -57,6 +56,7 @@ struct olpc_battery_data {
>  	struct power_supply *olpc_ac;
>  	struct power_supply *olpc_bat;
>  	char bat_serial[17];
> +	int new_proto;

bool?

-- Sebastian

>  };
>  
>  /*********************************************************************
> @@ -100,7 +100,7 @@ static const struct power_supply_desc olpc_ac_desc = {
>  static int olpc_bat_get_status(struct olpc_battery_data *data,
>  		union power_supply_propval *val, uint8_t ec_byte)
>  {
> -	if (olpc_platform_info.ecver > 0x44) {
> +	if (data->new_proto) {
>  		if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
>  			val->intval = POWER_SUPPLY_STATUS_CHARGING;
>  		else if (ec_byte & BAT_STAT_DISCHARGING)
> @@ -608,6 +608,7 @@ static int olpc_battery_probe(struct platform_device *pdev)
>  	struct power_supply_config psy_cfg = {};
>  	struct olpc_battery_data *data;
>  	uint8_t status;
> +	uint8_t ecver;
>  	int ret;
>  
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> @@ -615,13 +616,21 @@ static int olpc_battery_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, data);
>  
> -	/*
> -	 * We've seen a number of EC protocol changes; this driver requires
> -	 * the latest EC protocol, supported by 0x44 and above.
> -	 */
> -	if (olpc_platform_info.ecver < 0x44) {
> +	/* See if the EC is already there and get the EC revision */
> +	ret = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0, &ecver, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (ecver > 0x44) {
> +		/* XO 1 or 1.5 with a new EC firmware. */
> +		data->new_proto = 1;
> +	} else if (ecver < 0x44) {
> +		/*
> +		 * We've seen a number of EC protocol changes; this driver
> +		 * requires the latest EC protocol, supported by 0x44 and above.
> +		 */
>  		printk(KERN_NOTICE "OLPC EC version 0x%02x too old for "
> -			"battery driver.\n", olpc_platform_info.ecver);
> +			"battery driver.\n", ecver);
>  		return -ENXIO;
>  	}
>  
> -- 
> 2.20.1
> 

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

Re: [PATCH v5 7/7] power: supply: olpc_battery: Add OLPC XO 1.75 support

[Sebastian Reichel]

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

Hi,

On Thu, Jan 10, 2019 at 06:40:05PM +0100, Lubomir Rintel wrote:
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> Changes since v2:
> - Fix the version conditional
> 
> Changes since v1:
> - s/s16 ecword_to_cpu/u16 ecword_to_cpu/
> - s/u16 ec_byte/u16 ec_word/
> 
>  drivers/power/supply/olpc_battery.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index a6c89d002d5d..52de90049980 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -57,6 +57,7 @@ struct olpc_battery_data {
>  	struct power_supply *olpc_bat;
>  	char bat_serial[17];
>  	int new_proto;
> +	int little_endian;

bool?

Looks good otherwise.

-- Sebastian

>  };
>  
>  /*********************************************************************
> @@ -322,6 +323,14 @@ static int olpc_bat_get_voltage_max_design(union power_supply_propval *val)
>  	return ret;
>  }
>  
> +static u16 ecword_to_cpu(struct olpc_battery_data *data, u16 ec_word)
> +{
> +	if (data->little_endian)
> +		return le16_to_cpu(ec_word);
> +	else
> +		return be16_to_cpu(ec_word);
> +}
> +
>  /*********************************************************************
>   *		Battery properties
>   *********************************************************************/
> @@ -394,7 +403,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (s16)be16_to_cpu(ec_word) * 9760L / 32;
> +		val->intval = ecword_to_cpu(data, ec_word) * 9760L / 32;
>  		break;
>  	case POWER_SUPPLY_PROP_CURRENT_AVG:
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
> @@ -402,7 +411,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (s16)be16_to_cpu(ec_word) * 15625L / 120;
> +		val->intval = ecword_to_cpu(data, ec_word) * 15625L / 120;
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
>  		ret = olpc_ec_cmd(EC_BAT_SOC, NULL, 0, &ec_byte, 1);
> @@ -433,21 +442,21 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> +		val->intval = ecword_to_cpu(data, ec_word) * 10 / 256;
>  		break;
>  	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>  		ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> +		val->intval = (int)ecword_to_cpu(data, ec_word) * 10 / 256;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>  		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (s16)be16_to_cpu(ec_word) * 6250 / 15;
> +		val->intval = ecword_to_cpu(data, ec_word) * 6250 / 15;
>  		break;
>  	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
>  		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
> @@ -621,7 +630,11 @@ static int olpc_battery_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	if (ecver > 0x44) {
> +	if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
> +		/* XO 1.75 */
> +		data->new_proto = 1;
> +		data->little_endian = 1;
> +	} else if (ecver > 0x44) {
>  		/* XO 1 or 1.5 with a new EC firmware. */
>  		data->new_proto = 1;
>  	} else if (ecver < 0x44) {
> -- 
> 2.20.1
> 

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

Re: [PATCH v5 4/7] power: supply: olpc_battery: Move priv data to a struct

[Sebastian Reichel]

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

Hi,

On Thu, Jan 10, 2019 at 06:40:02PM +0100, Lubomir Rintel wrote:
> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

[...]

> -	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> +	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>  	if (ret)
>  		goto eeprom_failed;
>  
> -	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> +	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
>  	if (ret)
>  		goto error_failed;

Could you create another patch, that converts this to use the
functionality introduced in cef8fe6a382c? I missed the
device_create_* functions and you can actually test the change.
An example for such a conversion would be "711aebcfe3ba" (ds2781).

P.S.: This is not mandatory for applying this patchset of course.

-- Sebastian

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

Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node

[Sebastian Reichel]

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

Hi,

On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote:
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. Add a different compatible string to the 1.5
> battery.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> ---

I either need an Acked-by from the x86 platform maintainers, that I
can queue this through power-supply or a pull request for an immutable
branch (probably the better idea).

-- Sebastian

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

Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node

[Lubomir Rintel]

Hi,

On Wed, 2019-01-23 at 21:56 +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote:
> > The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> > ambient temperature. Add a different compatible string to the 1.5
> > battery.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > 
> > ---
> 
> I either need an Acked-by from the x86 platform maintainers, that I
> can queue this through power-supply or a pull request for an immutable
> branch (probably the better idea).

I'm happy to prepare a branch that could be pulled from. In fact,
here's a branch with fixes for issues pointed out by the review that
could be pulled from:

  git pull https://github.com/hackerspace/olpc-xo175-linux lr/olpc-xo175-battery-for-v5.1

What do really not understand is how does this help. This is probably
just my unfamiliarity with the process; but perhaps you could help me
get less unfamiliar. Would it somehow help with a potential (though
unlikely) conflict resolution? Would an Ack from x86 crowd serve as an
altenative way off making sure things in their tree won't conflict with
this one?

> -- Sebastian

Thank you
Lubo

Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node

[Borislav Petkov]

+ the platform maintainers.

On Thu, Jan 31, 2019 at 01:26:16PM +0100, Lubomir Rintel wrote:
> Hi,
> 
> On Wed, 2019-01-23 at 21:56 +0100, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote:
> > > The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> > > ambient temperature. Add a different compatible string to the 1.5
> > > battery.
> > > 
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > 
> > I either need an Acked-by from the x86 platform maintainers, that I
> > can queue this through power-supply or a pull request for an immutable
> > branch (probably the better idea).
> 
> I'm happy to prepare a branch that could be pulled from. In fact,
> here's a branch with fixes for issues pointed out by the review that
> could be pulled from:
> 
>   git pull https://github.com/hackerspace/olpc-xo175-linux lr/olpc-xo175-battery-for-v5.1
> 
> What do really not understand is how does this help. This is probably
> just my unfamiliarity with the process; but perhaps you could help me
> get less unfamiliar. Would it somehow help with a potential (though
> unlikely) conflict resolution? Would an Ack from x86 crowd serve as an
> altenative way off making sure things in their tree won't conflict with
> this one?
> 
> > -- Sebastian
> 
> Thank you
> Lubo
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node

[Lubomir Rintel]

Hi Sebastian,

perhaps the message slipped through the cracks? I'm happy to do
whatever is needed to get the patch set into 5.1, but it seems I need
some help and clarifications.

Thank you,
Lubo

On Thu, 2019-01-31 at 13:26 +0100, Lubomir Rintel wrote:
> Hi,
> 
> On Wed, 2019-01-23 at 21:56 +0100, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote:
> > > The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> > > ambient temperature. Add a different compatible string to the 1.5
> > > battery.
> > > 
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > 
> > I either need an Acked-by from the x86 platform maintainers, that I
> > can queue this through power-supply or a pull request for an immutable
> > branch (probably the better idea).
> 
> I'm happy to prepare a branch that could be pulled from. In fact,
> here's a branch with fixes for issues pointed out by the review that
> could be pulled from:
> 
>   git pull https://github.com/hackerspace/olpc-xo175-linux lr/olpc-xo175-battery-for-v5.1
> 
> What do really not understand is how does this help. This is probably
> just my unfamiliarity with the process; but perhaps you could help me
> get less unfamiliar. Would it somehow help with a potential (though
> unlikely) conflict resolution? Would an Ack from x86 crowd serve as an
> altenative way off making sure things in their tree won't conflict with
> this one?
> 
> > -- Sebastian
> 
> Thank you
> Lubo
> 

Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node

[Sebastian Reichel]

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

[+cc Darren Hart, Andy Shevchenko]

Hi Lubo,

On Mon, Feb 11, 2019 at 12:37:19PM +0100, Lubomir Rintel wrote:
> perhaps the message slipped through the cracks? I'm happy to do
> whatever is needed to get the patch set into 5.1, but it seems I
> need some help and clarifications.

The following patches should be merged through the power-supply
tree, which is maintained by me. This one is not for my subsystem,
so either I need an Acked-by from the x86 people (=they are ok
with me merging the patch through the power-supply tree) or they
merge it into the x86 platform tree. If it is merged through the
x86 tree I need a pull-request from them, so that I can merge the
other patches.

TLDR: This needs interaction from the x86 platform maintainers
(i.e. Darren Hart and Andy Shevchenko). Looks like you did not
Cc them?

https://patchwork.kernel.org/patch/10756459/

-- Sebastian

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

Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node

[Darren Hart]

On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote:
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. Add a different compatible string to the 1.5
> battery.

Hi Lubomir,

Thanks for the recent Cc and pointer to here. In general, I have no
problem with the net changes. I do have some concerns from a
reviewability and change documentation perspective.

You document your intent above, but you also made several other changes
to get there which aren't documented, so when reviewing they stand out
as "why is this here?".

From what I can tell, this change contains:

1) Cleanup of olpc_dt_interpret() calls to avoid splitting string
literals (noted in the patch history, but not called out as an
intentional change)

2) Refactoring of logic and breaking out the check for the compatible
property into olpc_dt_compatible_match

3) Addition of "we're running very old firmware if this is missing"
checks - I'm not sure how this relates to the stated problem and
the intended fix.

4) The addition of the xo1.5 compatible property.

All the other things makes it very difficult to determine if this patch
does what you describe - and only what you describe.

In general please:
a) Cleanup code
b) Refactor code
c) Change functionality

In that order - that way the new functionality is what show up when
someone does a git blame on the file (rather than a cleanup patch, which
isn't as useful in defect analysis).

And always document in your commit messages the approach you take to fix
the reported issue.

Thanks,


-- 
Darren Hart
VMware Open Source Technology Center