[PATCH 00/10] Add support for XO 1.75 to OLPC battery driver

[PATCH 00/10] 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.

Compared to the previous version, it splits the x86 platform patch into
three so that it's hopefully easier to review.

Patch 10/10 ("power: supply: olpc_battery: Have the framework register
sysfs files for us") is new. The review comments are addressed, details
in individual patches.

Thank you,
Lubo

[PATCH 01/10] 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 02/10] x86, olpc: Don't split string literals when fixing up the DT

[Lubomir Rintel]

It was pointed out in a review, and checkpatch.pl complains about this.
Breaking it down into multiple ofw evaluations works just as well, and
perhaps even reads better.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v5:
- This patch was split off from "x86, olpc: Use a correct version when
  making up a battery node"

 arch/x86/platform/olpc/olpc_dt.c | 33 ++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index b4ab779f1d47..b69c50cb9742 100644
--- a/arch/x86/platform/olpc/olpc_dt.c
+++ b/arch/x86/platform/olpc/olpc_dt.c
@@ -239,9 +239,9 @@ void __init olpc_dt_fixup(void)
 	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");
+	olpc_dt_interpret("\" /battery@0\" find-device");
+	olpc_dt_interpret("  \" olpc,xo1-battery\" +compatible");
+	olpc_dt_interpret("device-end");
 
 	board_rev = olpc_dt_get_board_revision();
 	if (!board_rev)
@@ -249,19 +249,24 @@ void __init olpc_dt_fixup(void)
 
 	if (board_rev >= olpc_board_pre(0xd0)) {
 		/* 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 {
 		/* 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");
 	}
 }
 
-- 
2.20.1

Re: [PATCH 02/10] x86, olpc: Don't split string literals when fixing up the DT

[Thomas Gleixner]

On Sun, 10 Mar 2019, Lubomir Rintel wrote:

Please use 'x86/platform/olpc:' as prefix in the subject.

> It was pointed out in a review, and checkpatch.pl complains about this.
> Breaking it down into multiple ofw evaluations works just as well, and
> perhaps even reads better.

perhaps?

Other than that:

Acked-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 03/10] x86, olpc: Trivial code move in DT fixup

[Lubomir Rintel]

This makes the following patch more concise.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v5:
- This patch was split off from "x86, olpc: Use a correct version when
  making up a battery node"

 arch/x86/platform/olpc/olpc_dt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index b69c50cb9742..adf98b5623c0 100644
--- a/arch/x86/platform/olpc/olpc_dt.c
+++ b/arch/x86/platform/olpc/olpc_dt.c
@@ -238,11 +238,6 @@ void __init olpc_dt_fixup(void)
 
 	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_dt_interpret("  \" olpc,xo1-battery\" +compatible");
-	olpc_dt_interpret("device-end");
-
 	board_rev = olpc_dt_get_board_revision();
 	if (!board_rev)
 		return;
@@ -268,6 +263,11 @@ void __init olpc_dt_fixup(void)
 		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

Re: [PATCH 03/10] x86, olpc: Trivial code move in DT fixup

[Thomas Gleixner]

On Sun, 10 Mar 2019, Lubomir Rintel wrote:

Same subject prefix as with previous patch please.

> This makes the following patch more concise.

Acked-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 04/10] 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. We need to use a different compatible string for the
XO-1.5 battery.

Previously olpc_dt_fixup() used the presence od the battery node's
compatible property to decide whether the DT is up to date. Now we need
to look for a particular value in the compatible string, to decide

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

---
Changes since v5:
- Split the "x86, olpc: Don't split string literals when fixing up the DT"
  and "x86, olpc: trivial code move in DT fixup" parts off from this
- Clarify some comments

Changes since v1:
- Avoid splitting string literals

 arch/x86/platform/olpc/olpc_dt.c | 64 +++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 14 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index adf98b5623c0..1728f8992850 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);
 }
 
+int olpc_dt_compatible_match(phandle node, const char *compat)
+{
+	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)
 {
-	int r;
-	char buf[64];
 	phandle node;
 	u32 board_rev;
 
@@ -228,22 +246,30 @@ 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");
-
 	board_rev = olpc_dt_get_board_revision();
 	if (!board_rev)
 		return;
 
 	if (board_rev >= olpc_board_pre(0xd0)) {
-		/* XO-1.5: add dcon device */
+		/* XO-1.5 */
+
+		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");
+
+		if (olpc_dt_compatible_match(node, "olpc,xo1-battery")) {
+			/* If we have a olpc,xo1-battery compatible, then we're
+			 * running a new enough firmware that already has
+			 * the dcon node.
+			 */
+			return;
+		}
+
+		/* Add dcon device */
 		olpc_dt_interpret("\" /pci/display@1\" find-device");
 		olpc_dt_interpret("  new-device");
 		olpc_dt_interpret("    \" dcon\" device-name");
@@ -251,7 +277,17 @@ void __init olpc_dt_fixup(void)
 		olpc_dt_interpret("  finish-device");
 		olpc_dt_interpret("device-end");
 	} else {
-		/* XO-1: add dcon device, mark RTC as olpc,xo1-rtc */
+		/* XO-1 */
+
+		if (olpc_dt_compatible_match(node, "olpc,xo1-battery")) {
+			/* If we have a olpc,xo1-battery compatible, then we're
+			 * running a new enough firmware that already has
+			 * the dcon and RTC nodes.
+			 */
+			return;
+		}
+
+		/* Add dcon device, mark RTC as olpc,xo1-rtc */
 		olpc_dt_interpret("\" /pci/display@1,1\" find-device");
 		olpc_dt_interpret("  new-device");
 		olpc_dt_interpret("    \" dcon\" device-name");
-- 
2.20.1

Re: [PATCH 04/10] x86, olpc: Use a correct version when making up a battery node

[Thomas Gleixner]

On Sun, 10 Mar 2019, Lubomir Rintel wrote:

Subject prefix ...

> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. We need to use a different compatible string for the
> XO-1.5 battery.
> 
> Previously olpc_dt_fixup() used the presence od the battery node's

s/od/of/

>  
> +int olpc_dt_compatible_match(phandle node, const char *compat)
> +{
> +	char buf[64];
> +	int plen;
> +	char *p;
> +	int len;

Please coalesce variables of the same type. No point in wasting space.

	char buf[64], *p;
	int plen, len;

Hmm?

> +
> +		if (olpc_dt_compatible_match(node, "olpc,xo1-battery")) {
> +			/* If we have a olpc,xo1-battery compatible, then we're
> +			 * running a new enough firmware that already has
> +			 * the dcon node.
> +			 */

Comment style:

       		 	 /*
			  * This is a proper multi line comment even
			  * if networking people use that horrible style
			  * above.
			  */

With those nitpicks fixed:

Acked-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH 04/10] x86, olpc: Use a correct version when making up a battery node

[Sebastian Reichel]

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

Hi,

On Sat, Mar 23, 2019 at 12:25:58AM +0100, Thomas Gleixner wrote:
> On Sun, 10 Mar 2019, Lubomir Rintel wrote:
> 
> Subject prefix ...
> 
> > The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> > ambient temperature. We need to use a different compatible string for the
> > XO-1.5 battery.
> > 
> > Previously olpc_dt_fixup() used the presence od the battery node's
> 
> s/od/of/
> 
> >  
> > +int olpc_dt_compatible_match(phandle node, const char *compat)
> > +{
> > +	char buf[64];
> > +	int plen;
> > +	char *p;
> > +	int len;
> 
> Please coalesce variables of the same type. No point in wasting space.
> 
> 	char buf[64], *p;
> 	int plen, len;
> 
> Hmm?
> 
> > +
> > +		if (olpc_dt_compatible_match(node, "olpc,xo1-battery")) {
> > +			/* If we have a olpc,xo1-battery compatible, then we're
> > +			 * running a new enough firmware that already has
> > +			 * the dcon node.
> > +			 */
> 
> Comment style:
> 
>        		 	 /*
> 			  * This is a proper multi line comment even
> 			  * if networking people use that horrible style
> 			  * above.
> 			  */
> 
> With those nitpicks fixed:
> 
> Acked-by: Thomas Gleixner <tglx@linutronix.de>

Looks like this is the last required change before this can be
merged. Assuming Lubomir sends a fixed series soon, how should
it be merged?

a) I get a pull-request with a immutable branch for patch 2-4
b) Complete patchset goes in via x86
c) Complete patchset goes in via power-supply

I'm fine with all variants.

-- Sebastian

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

[PATCH 05/10] 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 v5:
- Use of_device_is_compatible() instead of
  of_property_match_string("compatible")

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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 5a97e42a3547..3dcf5e19f329 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,12 @@ 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_device_is_compatible(pdev->dev.of_node, "olpc,xo1.5-battery")) {
+		/* 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 +674,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 06/10] 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 3dcf5e19f329..f7bb9bedd893 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_device_is_compatible(pdev->dev.of_node, "olpc,xo1.5-battery")) {
 		/* XO-1.5 */
 		olpc_bat_desc.properties = olpc_xo15_bat_props;
@@ -633,42 +648,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

Re: [PATCH 06/10] power: supply: olpc_battery: Move priv data to a struct

[Sebastian Reichel]

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

Hi,

On Sun, Mar 10, 2019 at 05:24:15PM +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>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> 
> ---
> 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 3dcf5e19f329..f7bb9bedd893 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_device_is_compatible(pdev->dev.of_node, "olpc,xo1.5-battery")) {
>  		/* XO-1.5 */
>  		olpc_bat_desc.properties = olpc_xo15_bat_props;
> @@ -633,42 +648,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
> 

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

[PATCH 07/10] 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 f7bb9bedd893..d83c77c2a0ec 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);
 
@@ -648,15 +648,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)
@@ -671,10 +669,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;
 }
 
@@ -684,9 +678,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 08/10] 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 v5:
- Turn new_proto into a bool

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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index d83c77c2a0ec..8be44c717d85 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_ac;
 	struct power_supply *olpc_bat;
 	char bat_serial[17];
+	bool new_proto;
 };
 
 /*********************************************************************
@@ -100,7 +101,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 +609,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 +617,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 = true;
+	} 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 09/10] 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 v5:
- s/int little_endian/bool little_endian/

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 8be44c717d85..0d67158c13d6 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -58,6 +58,7 @@ struct olpc_battery_data {
 	struct power_supply *olpc_bat;
 	char bat_serial[17];
 	bool new_proto;
+	bool little_endian;
 };
 
 /*********************************************************************
@@ -323,6 +324,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
  *********************************************************************/
@@ -395,7 +404,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:
@@ -403,7 +412,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);
@@ -434,21 +443,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);
@@ -622,7 +631,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 = true;
+		data->little_endian = true;
+	} else if (ecver > 0x44) {
 		/* XO 1 or 1.5 with a new EC firmware. */
 		data->new_proto = true;
 	} else if (ecver < 0x44) {
-- 
2.20.1

Re: [PATCH 09/10] power: supply: olpc_battery: Add OLPC XO 1.75 support

[Sebastian Reichel]

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

Hi,

On Sun, Mar 10, 2019 at 05:24:18PM +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>
> 
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> Changes since v5:
> - s/int little_endian/bool little_endian/
> 
> 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 8be44c717d85..0d67158c13d6 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -58,6 +58,7 @@ struct olpc_battery_data {
>  	struct power_supply *olpc_bat;
>  	char bat_serial[17];
>  	bool new_proto;
> +	bool little_endian;
>  };
>  
>  /*********************************************************************
> @@ -323,6 +324,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
>   *********************************************************************/
> @@ -395,7 +404,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:
> @@ -403,7 +412,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);
> @@ -434,21 +443,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);
> @@ -622,7 +631,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 = true;
> +		data->little_endian = true;
> +	} else if (ecver > 0x44) {
>  		/* XO 1 or 1.5 with a new EC firmware. */
>  		data->new_proto = true;
>  	} else if (ecver < 0x44) {
> -- 
> 2.20.1
> 

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

[PATCH 10/10] power: supply: olpc_battery: Have the framework register sysfs files for us

[Lubomir Rintel]

The power framework gained ability to register groups of sysfs
attributes in commit cef8fe6a382c ("power: supply: core: add support for
custom sysfs attributes").

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Suggested-by: Sebastian Reichel <sre@kernel.org>

---
Changes since v5:
- Added this patch

 drivers/power/supply/olpc_battery.c | 64 ++++++++++++++++-------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 0d67158c13d6..c628d3a47e14 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -551,7 +551,7 @@ static ssize_t olpc_bat_eeprom_read(struct file *filp, struct kobject *kobj,
 	return count;
 }
 
-static const struct bin_attribute olpc_bat_eeprom = {
+static struct bin_attribute olpc_bat_eeprom = {
 	.attr = {
 		.name = "eeprom",
 		.mode = S_IRUGO,
@@ -575,7 +575,7 @@ static ssize_t olpc_bat_error_read(struct device *dev,
 	return sprintf(buf, "%d\n", ec_byte);
 }
 
-static const struct device_attribute olpc_bat_error = {
+static struct device_attribute olpc_bat_error = {
 	.attr = {
 		.name = "error",
 		.mode = S_IRUGO,
@@ -583,6 +583,27 @@ static const struct device_attribute olpc_bat_error = {
 	.show = olpc_bat_error_read,
 };
 
+static struct attribute *olpc_bat_sysfs_attrs[] = {
+	&olpc_bat_error.attr,
+	NULL
+};
+
+static struct bin_attribute *olpc_bat_sysfs_bin_attrs[] = {
+	&olpc_bat_eeprom,
+	NULL
+};
+
+static const struct attribute_group olpc_bat_sysfs_group = {
+	.attrs = olpc_bat_sysfs_attrs,
+	.bin_attrs = olpc_bat_sysfs_bin_attrs,
+
+};
+
+static const struct attribute_group *olpc_bat_sysfs_groups[] = {
+	&olpc_bat_sysfs_group,
+	NULL
+};
+
 /*********************************************************************
  *		Initialisation
  *********************************************************************/
@@ -615,7 +636,8 @@ static int olpc_battery_suspend(struct platform_device *pdev,
 
 static int olpc_battery_probe(struct platform_device *pdev)
 {
-	struct power_supply_config psy_cfg = {};
+	struct power_supply_config bat_psy_cfg = {};
+	struct power_supply_config ac_psy_cfg = {};
 	struct olpc_battery_data *data;
 	uint8_t status;
 	uint8_t ecver;
@@ -654,10 +676,11 @@ static int olpc_battery_probe(struct platform_device *pdev)
 
 	/* Ignore the status. It doesn't actually matter */
 
-	psy_cfg.of_node = pdev->dev.of_node;
-	psy_cfg.drv_data = data;
+	ac_psy_cfg.of_node = pdev->dev.of_node;
+	ac_psy_cfg.drv_data = data;
 
-	data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
+	data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc,
+								&ac_psy_cfg);
 	if (IS_ERR(data->olpc_ac))
 		return PTR_ERR(data->olpc_ac);
 
@@ -671,37 +694,21 @@ static int olpc_battery_probe(struct platform_device *pdev)
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
 
-	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
+	bat_psy_cfg.of_node = pdev->dev.of_node;
+	bat_psy_cfg.drv_data = data;
+	bat_psy_cfg.attr_grp = olpc_bat_sysfs_groups;
+
+	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc,
+								&bat_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)
-		return ret;
-
-	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(&data->olpc_ac->dev, true);
 		device_set_wakeup_capable(&data->olpc_bat->dev, true);
 	}
 
 	return 0;
-
-error_failed:
-	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
-	return ret;
-}
-
-static int olpc_battery_remove(struct platform_device *pdev)
-{
-	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);
-	return 0;
 }
 
 static const struct of_device_id olpc_battery_ids[] = {
@@ -717,7 +724,6 @@ static struct platform_driver olpc_battery_driver = {
 		.of_match_table = olpc_battery_ids,
 	},
 	.probe = olpc_battery_probe,
-	.remove = olpc_battery_remove,
 	.suspend = olpc_battery_suspend,
 };
 
-- 
2.20.1

Re: [PATCH 10/10] power: supply: olpc_battery: Have the framework register sysfs files for us

[Sebastian Reichel]

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

Hi,

On Sun, Mar 10, 2019 at 05:24:19PM +0100, Lubomir Rintel wrote:
> The power framework gained ability to register groups of sysfs
> attributes in commit cef8fe6a382c ("power: supply: core: add support for
> custom sysfs attributes").
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Suggested-by: Sebastian Reichel <sre@kernel.org>
> 
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> Changes since v5:
> - Added this patch
> 
>  drivers/power/supply/olpc_battery.c | 64 ++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 0d67158c13d6..c628d3a47e14 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -551,7 +551,7 @@ static ssize_t olpc_bat_eeprom_read(struct file *filp, struct kobject *kobj,
>  	return count;
>  }
>  
> -static const struct bin_attribute olpc_bat_eeprom = {
> +static struct bin_attribute olpc_bat_eeprom = {
>  	.attr = {
>  		.name = "eeprom",
>  		.mode = S_IRUGO,
> @@ -575,7 +575,7 @@ static ssize_t olpc_bat_error_read(struct device *dev,
>  	return sprintf(buf, "%d\n", ec_byte);
>  }
>  
> -static const struct device_attribute olpc_bat_error = {
> +static struct device_attribute olpc_bat_error = {
>  	.attr = {
>  		.name = "error",
>  		.mode = S_IRUGO,
> @@ -583,6 +583,27 @@ static const struct device_attribute olpc_bat_error = {
>  	.show = olpc_bat_error_read,
>  };
>  
> +static struct attribute *olpc_bat_sysfs_attrs[] = {
> +	&olpc_bat_error.attr,
> +	NULL
> +};
> +
> +static struct bin_attribute *olpc_bat_sysfs_bin_attrs[] = {
> +	&olpc_bat_eeprom,
> +	NULL
> +};
> +
> +static const struct attribute_group olpc_bat_sysfs_group = {
> +	.attrs = olpc_bat_sysfs_attrs,
> +	.bin_attrs = olpc_bat_sysfs_bin_attrs,
> +
> +};
> +
> +static const struct attribute_group *olpc_bat_sysfs_groups[] = {
> +	&olpc_bat_sysfs_group,
> +	NULL
> +};
> +
>  /*********************************************************************
>   *		Initialisation
>   *********************************************************************/
> @@ -615,7 +636,8 @@ static int olpc_battery_suspend(struct platform_device *pdev,
>  
>  static int olpc_battery_probe(struct platform_device *pdev)
>  {
> -	struct power_supply_config psy_cfg = {};
> +	struct power_supply_config bat_psy_cfg = {};
> +	struct power_supply_config ac_psy_cfg = {};
>  	struct olpc_battery_data *data;
>  	uint8_t status;
>  	uint8_t ecver;
> @@ -654,10 +676,11 @@ static int olpc_battery_probe(struct platform_device *pdev)
>  
>  	/* Ignore the status. It doesn't actually matter */
>  
> -	psy_cfg.of_node = pdev->dev.of_node;
> -	psy_cfg.drv_data = data;
> +	ac_psy_cfg.of_node = pdev->dev.of_node;
> +	ac_psy_cfg.drv_data = data;
>  
> -	data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
> +	data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc,
> +								&ac_psy_cfg);
>  	if (IS_ERR(data->olpc_ac))
>  		return PTR_ERR(data->olpc_ac);
>  
> @@ -671,37 +694,21 @@ static int olpc_battery_probe(struct platform_device *pdev)
>  		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
>  	}
>  
> -	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> +	bat_psy_cfg.of_node = pdev->dev.of_node;
> +	bat_psy_cfg.drv_data = data;
> +	bat_psy_cfg.attr_grp = olpc_bat_sysfs_groups;
> +
> +	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc,
> +								&bat_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)
> -		return ret;
> -
> -	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(&data->olpc_ac->dev, true);
>  		device_set_wakeup_capable(&data->olpc_bat->dev, true);
>  	}
>  
>  	return 0;
> -
> -error_failed:
> -	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> -	return ret;
> -}
> -
> -static int olpc_battery_remove(struct platform_device *pdev)
> -{
> -	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);
> -	return 0;
>  }
>  
>  static const struct of_device_id olpc_battery_ids[] = {
> @@ -717,7 +724,6 @@ static struct platform_driver olpc_battery_driver = {
>  		.of_match_table = olpc_battery_ids,
>  	},
>  	.probe = olpc_battery_probe,
> -	.remove = olpc_battery_remove,
>  	.suspend = olpc_battery_suspend,
>  };
>  
> -- 
> 2.20.1
> 

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