olpc_battery: convert to platform device

This is needed so that suspend/resume (wakeup) support can be added.

Signed-off-by Daniel Drake <[email protected]>

---
 arch/x86/platform/olpc/olpc.c |   15 +++++++++++
 drivers/power/olpc_battery.c  |   53 +++++++++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 23 deletions(-)

v2: add MODULE_ALIAS thanks to Dmitry Torokhov

v3: fix __devexit annotation thanks to Dmitry

arch/x86/platform/olpc/olpc.c +15 -0

index edaf3fe..a5fb933 100644
@@ -239,6 +239,17 @@ static int __init add_xo1_platform_devices(void)
 	return 0;
 }
 
+static int __init add_common_platform_devices(void)
+{
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_simple("olpc-battery", -1, NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return 0;
+}
+
 static int __init olpc_init(void)
 {
 	int r = 0;

@@ -269,6 +280,10 @@ static int __init olpc_init(void)
 			olpc_platform_info.boardrev >> 4,
 			olpc_platform_info.ecver);
 
+	r = add_common_platform_devices();
+	if (r)
+		return r;
+
 	if (olpc_platform_info.boardrev < olpc_board_pre(0xd0)) { /* XO-1 */
 		r = add_xo1_platform_devices();
 		if (r)

drivers/power/olpc_battery.c +30 -23

index 0b0ff3a..18580b4 100644
@@ -519,8 +519,6 @@ static struct device_attribute olpc_bat_error = {
  *		Initialisation
  *********************************************************************/
 
-static struct platform_device *bat_pdev;
-
 static struct power_supply olpc_bat = {
 	.get_property = olpc_bat_get_property,
 	.use_for_apm = 1,

@@ -534,14 +532,11 @@ void olpc_battery_trigger_uevent(unsigned long cause)
 		kobject_uevent(&olpc_bat.dev->kobj, KOBJ_CHANGE);
 }
 
-static int __init olpc_bat_init(void)
+static int __devinit olpc_battery_probe(struct platform_device *pdev)
 {
-	int ret = 0;
+	int ret;
 	uint8_t status;
 
-	if (!olpc_platform_info.ecver)
-		return -ENXIO;
-
 	/*
 	 * We've seen a number of EC protocol changes; this driver requires
 	 * the latest EC protocol, supported by 0x44 and above.

@@ -552,21 +547,16 @@ static int __init olpc_bat_init(void)
 		return -ENXIO;
 	}
 
+	/* Ignore the status. It doesn't actually matter */
 	ret = olpc_ec_cmd(EC_BAT_STATUS, NULL, 0, &status, 1);
 	if (ret)
 		return ret;
 
-	/* Ignore the status. It doesn't actually matter */
-
-	bat_pdev = platform_device_register_simple("olpc-battery", 0, NULL, 0);
-	if (IS_ERR(bat_pdev))
-		return PTR_ERR(bat_pdev);
-
-	ret = power_supply_register(&bat_pdev->dev, &olpc_ac);
+	ret = power_supply_register(&pdev->dev, &olpc_ac);
 	if (ret)
-		goto ac_failed;
+		return ret;
 
-	olpc_bat.name = bat_pdev->name;
+	olpc_bat.name = pdev->name;
 	if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
 		olpc_bat.properties = olpc_xo15_bat_props;
 		olpc_bat.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);

@@ -575,7 +565,7 @@ static int __init olpc_bat_init(void)
 		olpc_bat.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
 
-	ret = power_supply_register(&bat_pdev->dev, &olpc_bat);
+	ret = power_supply_register(&pdev->dev, &olpc_bat);
 	if (ret)
 		goto battery_failed;
 

@@ -587,7 +577,7 @@ static int __init olpc_bat_init(void)
 	if (ret)
 		goto error_failed;
 
-	goto success;
+	return 0;
 
 error_failed:
 	device_remove_bin_file(olpc_bat.dev, &olpc_bat_eeprom);

@@ -595,19 +585,35 @@ eeprom_failed:
 	power_supply_unregister(&olpc_bat);
 battery_failed:
 	power_supply_unregister(&olpc_ac);
-ac_failed:
-	platform_device_unregister(bat_pdev);
-success:
 	return ret;
 }
 
-static void __exit olpc_bat_exit(void)
+static int __devexit 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);
-	platform_device_unregister(bat_pdev);
+	return 0;
+}
+
+static struct platform_driver olpc_battery_drv = {
+	.driver = {
+		.name = "olpc-battery",
+		.owner = THIS_MODULE,
+	},
+	.probe = olpc_battery_probe,
+	.remove = __devexit_p(olpc_battery_remove),
+};
+
+static int __init olpc_bat_init(void)
+{
+	return platform_driver_register(&olpc_battery_drv);
+}
+
+static void __exit olpc_bat_exit(void)
+{
+	platform_driver_unregister(&olpc_battery_drv);
 }
 
 module_init(olpc_bat_init);

@@ -616,3 +622,4 @@ module_exit(olpc_bat_exit);
 MODULE_AUTHOR("David Woodhouse <[email protected]>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Battery driver for One Laptop Per Child 'XO' machine");
+MODULE_ALIAS("platform:olpc-battery");

Signature · 1 line

1.7.4

arch/x86/platform/olpc/olpc.c static int __init add_xo1_platform_devices(void) · line 252

Quoted 12 lines · Daniel Drake

arch/x86/platform/olpc/olpc.c +11 -0

+static int __init add_common_platform_devices(void)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_register_simple("olpc-battery", -1, NULL, 0);
+       if (IS_ERR(pdev))
+               return PTR_ERR(pdev);
+
+       return 0;
+}
+ 

Still kind of sucks that you have to do this, and can't bind to something in the device-tree.

arch/x86/platform/olpc/olpc.c static int __init add_xo1_platform_devices(void) · line 252

Quoted 12 lines · Daniel Drake

arch/x86/platform/olpc/olpc.c +11 -0

+static int __init add_common_platform_devices(void)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_register_simple("olpc-battery", -1, NULL, 0);
+       if (IS_ERR(pdev))
+               return PTR_ERR(pdev);
+
+       return 0;
+}
+

Still kind of sucks that you have to do this, and can't bind to something in the device-tree.

Quoted 17 lines · David Woodhouse

Review

Quoted 13 lines · Daniel Drake

(quoted diff) +5 -0

+static int __init add_common_platform_devices(void)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_register_simple("olpc-battery", -1,

NULL, 0);
+       if (IS_ERR(pdev))
+               return PTR_ERR(pdev);

(quoted diff) +4 -0

+
+       return 0;
+}
+

Still kind of sucks that you have to do this, and can't bind to something in the device-tree.

OK, feel free to put this patch on hold for now. I started looking at the device tree approach today. It looks doable but first we have to fix a DT bug/inconsistency that is preventing us from correctly binding to the tree's devices.

Daniel

Quoted 25 lines · On Fri, 18 Feb 2011 23:42:57 +0000 Daniel Drake <[email protected]> wrote:

Quoted 17 lines · David Woodhouse

Review

Quoted 13 lines · Daniel Drake

(quoted diff) +11 -1

+static int __init add_common_platform_devices(void)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_register_simple("olpc-battery",
-1, NULL, 0);
+       if (IS_ERR(pdev))
+               return PTR_ERR(pdev);
+
+       return 0;
+}
+

Still kind of sucks that you have to do this, and can't bind to something in the device-tree.

OK, feel free to put this patch on hold for now. I started looking at the device tree approach today. It looks doable but first we have to fix a DT bug/inconsistency that is preventing us from correctly binding to the tree's devices.

Quoted 1 line

Daniel

Mea culpa. The patch below fixes a bug I introduced earlier. Cc'ing the sparc folks, as this probably affects them (although I would think that it fixes broken behavior for them..?)

Quoted 28 lines · On Fri, 18 Feb 2011 19:06:59 -0800 Andres Salomon <[email protected]> wrote:

Quoted 25 lines · On Fri, 18 Feb 2011 23:42:57 +0000 Daniel Drake <[email protected]> wrote:

Quoted 17 lines · David Woodhouse

Review

Quoted 13 lines · Daniel Drake

(quoted diff) +11 -1

+static int __init add_common_platform_devices(void)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_register_simple("olpc-battery",
-1, NULL, 0);
+       if (IS_ERR(pdev))
+               return PTR_ERR(pdev);
+
+       return 0;
+}
+

Still kind of sucks that you have to do this, and can't bind to something in the device-tree.

OK, feel free to put this patch on hold for now. I started looking at the device tree approach today. It looks doable but first we have to fix a DT bug/inconsistency that is preventing us from correctly binding to the tree's devices.

Quoted 9 lines

Quoted 1 line

Daniel

Mea culpa. The patch below fixes a bug I introduced earlier. Cc'ing the sparc folks, as this probably affects them (although I would think that it fixes broken behavior for them..?)

And this is a followup to the prior patch; an optimization based upon what we're doing with pkg2path stuff.

The sparc folks don't use pkg2path, so this shouldn't affect them at all.

Cc'ing Mitch as well, in case there's some reason why I should be wary of doing this. :)

From: Andres Salomon <[email protected]>

The of_pdt_try_pkg2path() function currently allocates memory for and fetches the full path name from OFW (of the form '/foo/bar/baz@0'). It then returns only the name ('baz@0)', and the caller re-constructs the full name (for dp->full_name) based upon dp->parent->full_name and what was returned by of_pdt_try_pkg2path(). Oh, and it allocates more memory for it.

OLPC is the only architecture which fills in the pkg2path hook, so this shouldn't affect any other users (ie, sparc). Since in practice (dp->parent->full_name + '/' + strrchr(pkg2path, '/')) and the result from pkg2path end up being exactly the same, just short circuit the rest of the of_pdt_build_full_name logic and set dp->full_name to the result of pkg2path. This means we don't have to parse it nor allocate more memory for it.

This saves time, code, and memory.

Signed-off-by Andres Salomon <[email protected]>

---
 drivers/of/pdt.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

drivers/of/pdt.c +2 -8

index b39d584..605834b 100644
@@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
 
 static char * __init of_pdt_try_pkg2path(phandle node)
 {
-	char *res, *buf = NULL;
+	char *buf = NULL;
 	int len;
 
 	if (!of_pdt_prom_ops->pkg2path)

@@ -147,13 +147,7 @@ static char * __init of_pdt_try_pkg2path(phandle node)
 		pr_err("%s: package-to-path failed\n", __func__);
 		return NULL;
 	}
-
-	res = strrchr(buf, '/');
-	if (!res) {
-		pr_err("%s: couldn't find / in %s\n", __func__, buf);
-		return NULL;
-	}
-	return res+1;
+	return buf;
 }
 
 static struct device_node * __init of_pdt_create_node(phandle node,

@@ -193,10 +187,13 @@ static char * __init of_pdt_build_full_name(struct device_node *dp,
 	 * name property (ie, 'battery'), we want the name we see with
 	 * package-to-path (ie, 'battery@0').

drivers/of/pdt.c +0 -0

 		name = dp->name;
 
 	plen = strlen(dp->parent->full_name);

Signature · 1 line

1.7.2.3

Quoted 25 lines · On Fri, 18 Feb 2011 23:42:57 +0000 Daniel Drake <[email protected]> wrote:

Quoted 17 lines · David Woodhouse

Review

Quoted 13 lines · Daniel Drake

(quoted diff) +5 -0

+static int __init add_common_platform_devices(void)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_register_simple("olpc-battery", -1,

NULL, 0);
+       if (IS_ERR(pdev))
+               return PTR_ERR(pdev);

(quoted diff) +4 -0

+
+       return 0;
+}
+

Still kind of sucks that you have to do this, and can't bind to something in the device-tree.

OK, feel free to put this patch on hold for now. I started looking at the device tree approach today. It looks doable but first we have to fix a DT bug/inconsistency that is preventing us from correctly binding to the tree's devices.

Quoted 1 line

Daniel

Mea culpa. The patch below fixes a bug I introduced earlier. Cc'ing the sparc folks, as this probably affects them (although I would think that it fixes broken behavior for them..?)

From: Andres Salomon <[email protected]>

Commit e2f2a93b changed dp->name from using the 'name' property to using package-to-path. This fixed /proc/device-tree creation by eliminating conflicts between names (the 'name' property provides names like 'battery', whereas package-to-path provides names like '/foo/bar/battery@0', which we stripped to 'battery@0'). However, it also breaks of_device_id table matching.

The fix that we _really_ wanted was to keep dp->name based upon the name property ('battery'), but based dp->full_name upon package-to-path ('battery@0'). This patch does just that.

Signed-off-by Andres Salomon <[email protected]>

Reported-by Daniel Drake <[email protected]>

---
 drivers/of/pdt.c |   42 +++++++++++++++++++-----------------------
 1 files changed, 19 insertions(+), 23 deletions(-)

drivers/of/pdt.c +0 -0

index 28295d0..b39d584 100644

drivers/of/pdt.c +5 -19

 #endif /* !CONFIG_SPARC */

@@ -156,23 +156,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
 	return res+1;
 }
 
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM callback,
- * or some other random failure), fall back to just looking at the node's
- * 'name' property.
- */
-static char * __init of_pdt_build_name(phandle node)
-{
-	char *buf;
-
-	buf = of_pdt_try_pkg2path(node);
-	if (!buf)
-		buf = of_pdt_get_one_property(node, "name");
-
-	return buf;
-}
-
 static struct device_node * __init of_pdt_create_node(phandle node,
 						    struct device_node *parent)
 {

@@ -187,7 +170,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
 
 	kref_init(&dp->kref);
 
-	dp->name = of_pdt_build_name(node);
+	dp->name = of_pdt_get_one_property(node, "name");
 	dp->type = of_pdt_get_one_property(node, "device_type");
 	dp->phandle = node;
 

@@ -198,13 +181,26 @@ static struct device_node * __init of_pdt_create_node(phandle node,
 	return dp;
 }
 
-static char * __init of_pdt_build_full_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp,
+		phandle node)
 {
 	int len, ourlen, plen;
+	const char *name;
 	char *n;
 
+	/*

drivers/of/pdt.c +3 -3

 	plen = strlen(dp->parent->full_name);
-	ourlen = strlen(of_pdt_node_name(dp));
+	ourlen = strlen(name);
 	len = ourlen + plen + 2;
 
 	n = prom_early_alloc(len);

@@ -213,7 +209,7 @@ static char * __init of_pdt_build_full_name(struct device_node *dp)
 		strcpy(n + plen, "/");
 		plen++;
 	}
-	strcpy(n + plen, of_pdt_node_name(dp));
+	strcpy(n + plen, name);
 
 	return n;
 }

@@ -243,7 +239,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
 #if defined(CONFIG_SPARC)
 		dp->path_component_name = build_path_component(dp);
 #endif
-		dp->full_name = of_pdt_build_full_name(dp);
+		dp->full_name = of_pdt_build_full_name(dp, node);
 
 		dp->child = of_pdt_build_tree(dp,
 				of_pdt_prom_ops->getchild(node), nextp);

Signature · 1 line

1.7.2.3

Quoted 31 lines · Andres Salomon

Quoted 26 lines · On Fri, 18 Feb 2011 23:42:57 +0000 Daniel Drake <[email protected]> wrote:

Quoted 17 lines · David Woodhouse

Review

Quoted 13 lines · Daniel Drake

(quoted diff) +11 -1

+static int __init add_common_platform_devices(void)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_register_simple("olpc-battery",
-1, NULL, 0);
+       if (IS_ERR(pdev))
+               return PTR_ERR(pdev);
+
+       return 0;
+}
+

Still kind of sucks that you have to do this, and can't bind to something in the device-tree.

OK, feel free to put this patch on hold for now. I started looking at the device tree approach today. It looks doable but first we have to fix a DT bug/inconsistency that is preventing us from correctly binding to the tree's devices.

Daniel

Quoted 3 lines

Mea culpa. The patch below fixes a bug I introduced earlier. Cc'ing the sparc folks, as this probably affects them (although I would think that it fixes broken behavior for them..?)

Wait; why are you binding to a device based on name? Binding by name and/or device_type is strongly discouraged for new code. Use compatible instead.

As for this patch, comments below...

Quoted 13 lines

From: Andres Salomon <[email protected]>

Commit e2f2a93b changed dp->name from using the 'name' property to using package-to-path. This fixed /proc/device-tree creation by eliminating conflicts between names (the 'name' property provides names like 'battery', whereas package-to-path provides names like '/foo/bar/battery@0', which we stripped to 'battery@0'). However, it also breaks of_device_id table matching.

The fix that we _really_ wanted was to keep dp->name based upon the name property ('battery'), but based dp->full_name upon package-to-path ('battery@0'). This patch does just that.

Quoted 2 lines

Signed-off-by Andres Salomon <[email protected]>

Reported-by Daniel Drake <[email protected]>

From what I can tell, this only affects OLPC, correct? It looks like SPARC's implementation of of_pdt_node_name() will sidestep most of this name retrieval code.

Quoted 4 lines

---
 drivers/of/pdt.c |   42 +++++++++++++++++++-----------------------
 1 files changed, 19 insertions(+), 23 deletions(-)

drivers/of/pdt.c static inline void irq_trans_init(struct · line 52

Quoted 11 lines

drivers/of/pdt.c +1 -1

index 28295d0..b39d584 100644
@@ -48,7 +48,7 @@ static inline void irq_trans_init(struct
 device_node *dp) { } 
 static inline const char *of_pdt_node_name(struct device_node *dp)
 {
-	return dp->name;
+	return NULL;
 }

Rather than using this hook; perhaps the sparc .path_component_name should be enabled for all architectures. It is a useful data item to keep a pointer to, and it would simplify the of_pdt code.

Quoted 1 line

Quoted 4 lines

Wait; why are you binding to a device based on name?  Binding by name and/or device_type is strongly discouraged for new code.  Use compatible instead.

Daniel posted a separate patch showing his code, would you mind commenting on that?  I noticed he didn't cc you though, here's the patch:

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

Quoted 9 lines · Andres Salomon

Quoted 4 lines

Wait; why are you binding to a device based on name?  Binding by name and/or device_type is strongly discouraged for new code.  Use compatible instead.

Daniel posted a separate patch showing his code, would you mind commenting on that?  I noticed he didn't cc you though, here's the patch:

Quoted 1 line

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

It does bind to the name.
We don't currently have a compatible property for the battery device. We could fix that with a firmware upgrade, but it would break compatibility with all existing installs. I guess this is still the recommended approach...

Quoted 5 lines · On Wed, 23 Feb 2011 12:43:52 -0700 Grant Likely <[email protected]>…

On Fri, Feb 18, 2011 at 07:06:59PM -0800, Andres Salomon wrote:
Wait; why are you binding to a device based on name? Binding by name and/or device_type is strongly discouraged for new code. Use compatible instead.

Daniel posted a separate patch showing his code, would you mind commenting on that? I noticed he didn't cc you though, here's the patch:

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