[PATCH] olpc_battery: bind to device tree

[PATCH] olpc_battery: bind to device tree

[Daniel Drake]

Move from being statically probed by platform presence to binding to
the battery node of the device tree.

This is cleaner and allows suspend/resume (wakeup) support to be added
in a later patch.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 arch/x86/platform/olpc/olpc_dt.c |   13 +++++++
 drivers/power/Kconfig            |    2 +-
 drivers/power/olpc_battery.c     |   66 +++++++++++++++++++++++++------------
 3 files changed, 58 insertions(+), 23 deletions(-)

Replaces patch "olpc_battery: convert to platform device"

The problem mentioned in another mail regarding devicetree binding is now
fixed via separate patches from Andres.

diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index dab8746..97211c1 100644
--- a/arch/x86/platform/olpc/olpc_dt.c
+++ b/arch/x86/platform/olpc/olpc_dt.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/bootmem.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/of_pdt.h>
 #include <asm/olpc_ofw.h>
 
@@ -181,3 +182,15 @@ void __init olpc_dt_build_devicetree(void)
 	pr_info("PROM DT: Built device tree with %u bytes of memory.\n",
 			prom_early_allocated);
 }
+
+/* A list of DT node/bus matches that we want to expose as platform devices */
+static struct of_device_id __initdata of_ids[] = {
+	{ .name = "battery" },
+	{},
+};
+
+static int __init declare_of_platform_devices(void)
+{
+	return of_platform_bus_probe(NULL, of_ids, NULL);
+}
+device_initcall(declare_of_platform_devices);
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 61bf5d7..3a9151d 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -84,7 +84,7 @@ config BATTERY_PMU
 
 config BATTERY_OLPC
 	tristate "One Laptop Per Child battery"
-	depends on X86_32 && OLPC
+	depends on X86_32 && OLPC && OF
 	help
 	  Say Y to enable support for the battery on the OLPC laptop.
 
diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
index 0b0ff3a..99fc8e0 100644
--- a/drivers/power/olpc_battery.c
+++ b/drivers/power/olpc_battery.c
@@ -17,6 +17,7 @@
 #include <linux/power_supply.h>
 #include <linux/jiffies.h>
 #include <linux/sched.h>
+#include <linux/of_platform.h>
 #include <asm/olpc.h>
 
 
@@ -519,9 +520,8 @@ static struct device_attribute olpc_bat_error = {
  *		Initialisation
  *********************************************************************/
 
-static struct platform_device *bat_pdev;
-
 static struct power_supply olpc_bat = {
+	.name = "olpc-battery",
 	.get_property = olpc_bat_get_property,
 	.use_for_apm = 1,
 };
@@ -534,13 +534,18 @@ 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,
+					const struct of_device_id *match)
 {
-	int ret = 0;
+	int ret;
 	uint8_t status;
+	struct device_node *root;
+	const char *arch;
+	int propsize;
 
-	if (!olpc_platform_info.ecver)
-		return -ENXIO;
+	/* Check that we're running on an XO laptop */
+	if (!machine_is_olpc())
+		return -ENODEV;
 
 	/*
 	 * We've seen a number of EC protocol changes; this driver requires
@@ -552,21 +557,15 @@ 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;
 	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 +574,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 +586,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 +594,42 @@ 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 const struct of_device_id olpc_battery_ids[] __devinitconst = {
+	{ .name = "battery" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, olpc_battery_ids);
+
+static struct of_platform_driver olpc_battery_drv = {
+	.driver = {
+		.name = "olpc-battery",
+		.owner = THIS_MODULE,
+		.of_match_table = olpc_battery_ids,
+	},
+	.probe = olpc_battery_probe,
+	.remove = __devexit_p(olpc_battery_remove),
+};
+
+static int __init olpc_bat_init(void)
+{
+	return of_register_platform_driver(&olpc_battery_drv);
+}
+
+static void __exit olpc_bat_exit(void)
+{
+	of_unregister_platform_driver(&olpc_battery_drv);
 }
 
 module_init(olpc_bat_init);
-- 
1.7.4

Re: [PATCH] olpc_battery: bind to device tree

[Grant Likely]

[cc'ing devicetree-discuss@lists.ozlabs.org.  Please cc this list for
any dt-related patches]

On Sat, Feb 19, 2011 at 8:06 AM, Daniel Drake <dsd@laptop.org> wrote:
> Move from being statically probed by platform presence to binding to
> the battery node of the device tree.
>
> This is cleaner and allows suspend/resume (wakeup) support to be added
> in a later patch.
>
> Signed-off-by: Daniel Drake <dsd@laptop.org>

Hi Daniel, comments below.

> ---
>  arch/x86/platform/olpc/olpc_dt.c |   13 +++++++
>  drivers/power/Kconfig            |    2 +-
>  drivers/power/olpc_battery.c     |   66 +++++++++++++++++++++++++------------
>  3 files changed, 58 insertions(+), 23 deletions(-)
>
> Replaces patch "olpc_battery: convert to platform device"
>
> The problem mentioned in another mail regarding devicetree binding is now
> fixed via separate patches from Andres.
>
> diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
> index dab8746..97211c1 100644
> --- a/arch/x86/platform/olpc/olpc_dt.c
> +++ b/arch/x86/platform/olpc/olpc_dt.c
> @@ -19,6 +19,7 @@
>  #include <linux/kernel.h>
>  #include <linux/bootmem.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/of_pdt.h>
>  #include <asm/olpc_ofw.h>
>
> @@ -181,3 +182,15 @@ void __init olpc_dt_build_devicetree(void)
>        pr_info("PROM DT: Built device tree with %u bytes of memory.\n",
>                        prom_early_allocated);
>  }
> +
> +/* A list of DT node/bus matches that we want to expose as platform devices */
> +static struct of_device_id __initdata of_ids[] = {
> +       { .name = "battery" },
> +       {},

As mentioned in the other thread, matching by name is strongly
discouraged.  It isn't very accurate and compatible is the preferred
method for binding devices.  'battery' in particular is highly
non-specific.

I do understand that you don't have a compatible property in the
current firmware, and to a certain extent we have to live with what
we're given by the kernel.  However, I think it would be better in the
OLPC case to find the battery node and add a compatible property
before registering a platform_device for it.  (or use a bus notifier
to tell you when it is registered, and add 'compatible' at that
point.)  That way we the uncertainty is taken care of in the board
support code without polluting the driver matching namespace.

> +};
> +
> +static int __init declare_of_platform_devices(void)
> +{
> +       return of_platform_bus_probe(NULL, of_ids, NULL);
> +}
> +device_initcall(declare_of_platform_devices);
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 61bf5d7..3a9151d 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -84,7 +84,7 @@ config BATTERY_PMU
>
>  config BATTERY_OLPC
>        tristate "One Laptop Per Child battery"
> -       depends on X86_32 && OLPC
> +       depends on X86_32 && OLPC && OF
>        help
>          Say Y to enable support for the battery on the OLPC laptop.
>
> diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
> index 0b0ff3a..99fc8e0 100644
> --- a/drivers/power/olpc_battery.c
> +++ b/drivers/power/olpc_battery.c
> @@ -17,6 +17,7 @@
>  #include <linux/power_supply.h>
>  #include <linux/jiffies.h>
>  #include <linux/sched.h>
> +#include <linux/of_platform.h>
>  #include <asm/olpc.h>
>
>
> @@ -519,9 +520,8 @@ static struct device_attribute olpc_bat_error = {
>  *             Initialisation
>  *********************************************************************/
>
> -static struct platform_device *bat_pdev;
> -
>  static struct power_supply olpc_bat = {
> +       .name = "olpc-battery",
>        .get_property = olpc_bat_get_property,
>        .use_for_apm = 1,
>  };
> @@ -534,13 +534,18 @@ 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,
> +                                       const struct of_device_id *match)

static int __devinit olpc_battery_probe(struct platform_device *pdev)

match is no longer needed (see below)

>  {
> -       int ret = 0;
> +       int ret;
>        uint8_t status;
> +       struct device_node *root;
> +       const char *arch;
> +       int propsize;
>
> -       if (!olpc_platform_info.ecver)
> -               return -ENXIO;
> +       /* Check that we're running on an XO laptop */
> +       if (!machine_is_olpc())
> +               return -ENODEV;
>
>        /*
>         * We've seen a number of EC protocol changes; this driver requires
> @@ -552,21 +557,15 @@ 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;
>        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 +574,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 +586,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 +594,42 @@ 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 const struct of_device_id olpc_battery_ids[] __devinitconst = {
> +       { .name = "battery" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> +
> +static struct of_platform_driver olpc_battery_drv = {

This should be:

static struct platform_driver olpc_battery_drv = {

of_platform_driver is deprecated and in the process of being removed.
platform_driver is pretty much a drop in replacement as long as the
.of_match_table is populated.

> +       .driver = {
> +               .name = "olpc-battery",
> +               .owner = THIS_MODULE,
> +               .of_match_table = olpc_battery_ids,
> +       },
> +       .probe = olpc_battery_probe,
> +       .remove = __devexit_p(olpc_battery_remove),
> +};
> +
> +static int __init olpc_bat_init(void)
> +{
> +       return of_register_platform_driver(&olpc_battery_drv);

return platform_driver_register(&olpc_battery_drv)

> +}
> +
> +static void __exit olpc_bat_exit(void)
> +{
> +       of_unregister_platform_driver(&olpc_battery_drv);

platform_driver_unregister()

>  }
>
>  module_init(olpc_bat_init);
> --
> 1.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] olpc_battery: bind to device tree

[Daniel Drake]

On 23 February 2011 20:34, Grant Likely <grant.likely@secretlab.ca> wrote:
> As mentioned in the other thread, matching by name is strongly
> discouraged.  It isn't very accurate and compatible is the preferred
> method for binding devices.  'battery' in particular is highly
> non-specific.
>
> I do understand that you don't have a compatible property in the
> current firmware, and to a certain extent we have to live with what
> we're given by the kernel.  However, I think it would be better in the
> OLPC case to find the battery node and add a compatible property
> before registering a platform_device for it.  (or use a bus notifier
> to tell you when it is registered, and add 'compatible' at that
> point.)  That way we the uncertainty is taken care of in the board
> support code without polluting the driver matching namespace.

Thanks for the review. This and the rest of your feedback makes sense.

Would you mind commenting on exactly how this should look?

Here are the changes i'm planning to make, both to the firmware and
with kernel code as you suggest to fixup device trees for systems with
old firmware:

/battery@0/compatible property added with value "olpc-battery" (XO-1 and XO-1.5)
/pci/isa@f/rtc@i70/compatible property prepended with "olpc-xo1-rtc,"
(XO-1 only)
/pci/display@1,1/dcon device added, name=dcon compatible=olpc-dcon
(XO-1 and XO-1.5)

In addition to the battery patch you reviewed, we plan to make the
olpc-rtc and DCON drivers bind to device tree nodes, which is the
reason behind the other changes.

Daniel

Re: [PATCH] olpc_battery: bind to device tree

[Daniel Drake]

On 25 February 2011 14:19, Daniel Drake <dsd@laptop.org> wrote:
> /battery@0/compatible property added with value "olpc-battery" (XO-1 and XO-1.5)
> /pci/isa@f/rtc@i70/compatible property prepended with "olpc-xo1-rtc,"

That comma at the end should be dropped. I just realised that the
separator between different elements in the compatible list is a NULL
character.

Re: [PATCH] olpc_battery: bind to device tree

[Grant Likely]

On Fri, Feb 25, 2011 at 7:19 AM, Daniel Drake <dsd@laptop.org> wrote:
> On 23 February 2011 20:34, Grant Likely <grant.likely@secretlab.ca> wrote:
>> As mentioned in the other thread, matching by name is strongly
>> discouraged.  It isn't very accurate and compatible is the preferred
>> method for binding devices.  'battery' in particular is highly
>> non-specific.
>>
>> I do understand that you don't have a compatible property in the
>> current firmware, and to a certain extent we have to live with what
>> we're given by the kernel.  However, I think it would be better in the
>> OLPC case to find the battery node and add a compatible property
>> before registering a platform_device for it.  (or use a bus notifier
>> to tell you when it is registered, and add 'compatible' at that
>> point.)  That way we the uncertainty is taken care of in the board
>> support code without polluting the driver matching namespace.
>
> Thanks for the review. This and the rest of your feedback makes sense.
>
> Would you mind commenting on exactly how this should look?

Before calling of_platform_bus_probe(), search the device tree for the
battery node and use prom_add_property() to add a compatible prop.

>
> Here are the changes i'm planning to make, both to the firmware and
> with kernel code as you suggest to fixup device trees for systems with
> old firmware:
>
> /battery@0/compatible property added with value "olpc-battery" (XO-1 and XO-1.5)
> /pci/isa@f/rtc@i70/compatible property prepended with "olpc-xo1-rtc,"
> (XO-1 only)
> /pci/display@1,1/dcon device added, name=dcon compatible=olpc-dcon
> (XO-1 and XO-1.5)

Compatible properties should generally be in the form:
"<vendor>,<part>".  So these should probably be:

battery:  compatible = "olpc,xo1-battery";
rtc: compatible = "olpc,xo1-rtc";
dcon: compatible = "olpc,xo1-dcon";

I would explicitly specify the "xo1-" part in all three to protect
against the eventuality of a new xo revision that has does something
incompatible.  ie. "olpc-battery" doesn't help if, say, XO-1.75 has a
different battery.  Newer configurations can claim compatibility with
the old if they are truly compatible.

>
> In addition to the battery patch you reviewed, we plan to make the
> olpc-rtc and DCON drivers bind to device tree nodes, which is the
> reason behind the other changes.

okay.

g.

Re: [PATCH] olpc_battery: bind to device tree

[Anton Vorontsov]

On Wed, Feb 23, 2011 at 01:34:30PM -0700, Grant Likely wrote:
> [cc'ing devicetree-discuss@lists.ozlabs.org.  Please cc this list for
> any dt-related patches]
> 
> On Sat, Feb 19, 2011 at 8:06 AM, Daniel Drake <dsd@laptop.org> wrote:
> > Move from being statically probed by platform presence to binding to
> > the battery node of the device tree.
> >
> > This is cleaner and allows suspend/resume (wakeup) support to be added
> > in a later patch.
> >
> > Signed-off-by: Daniel Drake <dsd@laptop.org>
> 
> Hi Daniel, comments below.

Daniel,

I just wondering what happened with this patch?..

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH] olpc_battery: bind to device tree

[Daniel Drake]

On 20 May 2011 16:21, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> I just wondering what happened with this patch?..

I sneakily posted it for review "prematurely" :)

I have a corrected version based upon all received feedback, but this
patch actually depends on another patch (for x86 tree) which has been
submitted and resubmitted and is still awaiting review:

http://marc.info/?l=linux-kernel&m=130416849500918&w=2

Haven't forgotten, but thanks for the reminder :)

Daniel