[PATCH 1/2] Driver core: use kmemdup in platform_device_add_resources

[PATCH 1/2] Driver core: use kmemdup in platform_device_add_resources

[Uwe Kleine-König]

This makes platform_device_add_resources look like
platform_device_add_data.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4d99c8b..26eb69d 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -191,13 +191,13 @@ int platform_device_add_resources(struct platform_device *pdev,
 {
 	struct resource *r;
 
-	r = kmalloc(sizeof(struct resource) * num, GFP_KERNEL);
+	r = kmemdup(res, sizeof(struct resource) * num, GFP_KERNEL);
 	if (r) {
-		memcpy(r, res, sizeof(struct resource) * num);
 		pdev->resource = r;
 		pdev->num_resources = num;
+		return 0;
 	}
-	return r ? 0 : -ENOMEM;
+	return -ENOMEM;
 }
 EXPORT_SYMBOL_GPL(platform_device_add_resources);
 
-- 
1.7.1

[PATCH 2/2] Driver core: reduce duplicated code

[Uwe Kleine-König]

This makes the two similar functions platform_device_register_simple
and platform_device_register_data one line inline functions using a new
generic function platform_device_register_resndata.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c         |  102 ++++++++++-----------------------------
 include/linux/platform_device.h |   62 ++++++++++++++++++++++--
 2 files changed, 83 insertions(+), 81 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 26eb69d..23a5853 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -344,108 +344,56 @@ void platform_device_unregister(struct platform_device *pdev)
 EXPORT_SYMBOL_GPL(platform_device_unregister);
 
 /**
- * platform_device_register_simple - add a platform-level device and its resources
- * @name: base name of the device we're adding
- * @id: instance id
- * @res: set of resources that needs to be allocated for the device
- * @num: number of resources
- *
- * This function creates a simple platform device that requires minimal
- * resource and memory management. Canned release function freeing memory
- * allocated for the device allows drivers using such devices to be
- * unloaded without waiting for the last reference to the device to be
- * dropped.
+ * platform_device_register_resndata - add a platform-level device with
+ * resources and platform-specific data
  *
- * This interface is primarily intended for use with legacy drivers which
- * probe hardware directly.  Because such drivers create sysfs device nodes
- * themselves, rather than letting system infrastructure handle such device
- * enumeration tasks, they don't fully conform to the Linux driver model.
- * In particular, when such drivers are built as modules, they can't be
- * "hotplugged".
- *
- * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
- */
-struct platform_device *platform_device_register_simple(const char *name,
-							int id,
-							const struct resource *res,
-							unsigned int num)
-{
-	struct platform_device *pdev;
-	int retval;
-
-	pdev = platform_device_alloc(name, id);
-	if (!pdev) {
-		retval = -ENOMEM;
-		goto error;
-	}
-
-	if (num) {
-		retval = platform_device_add_resources(pdev, res, num);
-		if (retval)
-			goto error;
-	}
-
-	retval = platform_device_add(pdev);
-	if (retval)
-		goto error;
-
-	return pdev;
-
-error:
-	platform_device_put(pdev);
-	return ERR_PTR(retval);
-}
-EXPORT_SYMBOL_GPL(platform_device_register_simple);
-
-/**
- * platform_device_register_data - add a platform-level device with platform-specific data
  * @parent: parent device for the device we're adding
  * @name: base name of the device we're adding
  * @id: instance id
+ * @res: set of resources that needs to be allocated for the device
+ * @num: number of resources
  * @data: platform specific data for this platform device
  * @size: size of platform specific data
  *
- * This function creates a simple platform device that requires minimal
- * resource and memory management. Canned release function freeing memory
- * allocated for the device allows drivers using such devices to be
- * unloaded without waiting for the last reference to the device to be
- * dropped.
- *
  * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
  */
-struct platform_device *platform_device_register_data(
+struct platform_device *platform_device_register_resndata(
 		struct device *parent,
 		const char *name, int id,
+		const struct resource *res, unsigned int num,
 		const void *data, size_t size)
 {
+	int ret = -ENOMEM;
 	struct platform_device *pdev;
-	int retval;
 
 	pdev = platform_device_alloc(name, id);
-	if (!pdev) {
-		retval = -ENOMEM;
-		goto error;
-	}
+	if (!pdev)
+		goto err;
 
 	pdev->dev.parent = parent;
 
+	if (num) {
+		ret = platform_device_add_resources(pdev, res, num);
+		if (ret)
+			goto err;
+	}
+
 	if (size) {
-		retval = platform_device_add_data(pdev, data, size);
-		if (retval)
-			goto error;
+		ret = platform_device_add_data(pdev, data, size);
+		if (ret)
+			goto err;
 	}
 
-	retval = platform_device_add(pdev);
-	if (retval)
-		goto error;
+	ret = platform_device_add(pdev);
+	if (ret) {
+err:
+		platform_device_put(pdev);
+		return ERR_PTR(ret);
+	}
 
 	return pdev;
-
-error:
-	platform_device_put(pdev);
-	return ERR_PTR(retval);
 }
-EXPORT_SYMBOL_GPL(platform_device_register_data);
+EXPORT_SYMBOL_GPL(platform_device_register_resndata);
 
 static int platform_drv_probe(struct device *_dev)
 {
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 5417944..d7ecad0 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -43,10 +43,64 @@ extern struct resource *platform_get_resource_byname(struct platform_device *, u
 extern int platform_get_irq_byname(struct platform_device *, const char *);
 extern int platform_add_devices(struct platform_device **, int);
 
-extern struct platform_device *platform_device_register_simple(const char *, int id,
-					const struct resource *, unsigned int);
-extern struct platform_device *platform_device_register_data(struct device *,
-		const char *, int, const void *, size_t);
+extern struct platform_device *platform_device_register_resndata(
+		struct device *parent, const char *name, int id,
+		const struct resource *res, unsigned int num,
+		const void *data, size_t size);
+
+/**
+ * platform_device_register_simple - add a platform-level device and its resources
+ * @name: base name of the device we're adding
+ * @id: instance id
+ * @res: set of resources that needs to be allocated for the device
+ * @num: number of resources
+ *
+ * This function creates a simple platform device that requires minimal
+ * resource and memory management. Canned release function freeing memory
+ * allocated for the device allows drivers using such devices to be
+ * unloaded without waiting for the last reference to the device to be
+ * dropped.
+ *
+ * This interface is primarily intended for use with legacy drivers which
+ * probe hardware directly.  Because such drivers create sysfs device nodes
+ * themselves, rather than letting system infrastructure handle such device
+ * enumeration tasks, they don't fully conform to the Linux driver model.
+ * In particular, when such drivers are built as modules, they can't be
+ * "hotplugged".
+ *
+ * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
+ */
+static inline struct platform_device *platform_device_register_simple(
+		const char *name, int id,
+		const struct resource *res, unsigned int num)
+{
+	return platform_device_register_resndata(NULL, name, id,
+			res, num, NULL, 0);
+}
+
+/**
+ * platform_device_register_data - add a platform-level device with platform-specific data
+ * @parent: parent device for the device we're adding
+ * @name: base name of the device we're adding
+ * @id: instance id
+ * @data: platform specific data for this platform device
+ * @size: size of platform specific data
+ *
+ * This function creates a simple platform device that requires minimal
+ * resource and memory management. Canned release function freeing memory
+ * allocated for the device allows drivers using such devices to be
+ * unloaded without waiting for the last reference to the device to be
+ * dropped.
+ *
+ * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
+ */
+static inline struct platform_device *platform_device_register_data(
+		struct device *parent, const char *name, int id,
+		const void *data, size_t size)
+{
+	return platform_device_register_resndata(parent, name, id,
+			NULL, 0, data, size);
+}
 
 extern struct platform_device *platform_device_alloc(const char *name, int id);
 extern int platform_device_add_resources(struct platform_device *pdev,
-- 
1.7.1

Re: [PATCH 2/2] Driver core: reduce duplicated code

[Uwe Kleine-König]

Hello,

On Tue, Jun 15, 2010 at 10:47:56AM +0200, Uwe Kleine-König wrote:
> This makes the two similar functions platform_device_register_simple
> and platform_device_register_data one line inline functions using a new
> generic function platform_device_register_resndata.
I forgot to add some comments to this mail, ... sorry.

 - I'm not completely happy with the name of the new function.  If
   someone has a better name please tell me.
 - can platform_device_register_resndata be moved to __init_or_module?
 - I moved the kernel docs to the header but didn't test if they are
   picked up when generating docs.  Even if not, there is no better
   place, is there?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] Driver core: reduce duplicated code

[Greg KH]

On Tue, Jun 15, 2010 at 11:05:00AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jun 15, 2010 at 10:47:56AM +0200, Uwe Kleine-König wrote:
> > This makes the two similar functions platform_device_register_simple
> > and platform_device_register_data one line inline functions using a new
> > generic function platform_device_register_resndata.
> I forgot to add some comments to this mail, ... sorry.
> 
>  - I'm not completely happy with the name of the new function.  If
>    someone has a better name please tell me.

I don't like it either, what is "resndata" supposed to stand for?

>  - can platform_device_register_resndata be moved to __init_or_module?

I doubt it, but try it and see if a build warns about it.

>  - I moved the kernel docs to the header but didn't test if they are
>    picked up when generating docs.  Even if not, there is no better
>    place, is there?

No, that's the proper place, but make sure the docbook source is also
picking up the .h file, I don't know if it currently does.

So, I'll not apply this one just yet.  Can you verify the docbook stuff
at the very least?

thanks,

greg k-h

Re: [PATCH 2/2] Driver core: reduce duplicated code

[Uwe Kleine-König]

Hi Greg,

On Wed, Jun 16, 2010 at 01:53:32PM -0700, Greg KH wrote:
> On Tue, Jun 15, 2010 at 11:05:00AM +0200, Uwe Kleine-König wrote:
> > On Tue, Jun 15, 2010 at 10:47:56AM +0200, Uwe Kleine-König wrote:
> > > This makes the two similar functions platform_device_register_simple
> > > and platform_device_register_data one line inline functions using a new
> > > generic function platform_device_register_resndata.
> > I forgot to add some comments to this mail, ... sorry.
> > 
> >  - I'm not completely happy with the name of the new function.  If
> >    someone has a better name please tell me.
> 
> I don't like it either, what is "resndata" supposed to stand for?
resources and data -> res'n'data -> resndata.

> >  - can platform_device_register_resndata be moved to __init_or_module?
> 
> I doubt it, but try it and see if a build warns about it.
for x86_64_defconfig + MODULES=n there are two section mismatches:
	dock_add
	regulatory_init

regulatory_init is only called from

	static int __init cfg80211_init(void)

.  (I just sent a patch[1] that moves regulatory_init to .init.text.)

dock_add (in drivers/acpi/dock.c) is a bit harder.  I will take a look
later if it can go to .init.text (together with a few more functions),
too.

> >  - I moved the kernel docs to the header but didn't test if they are
> >    picked up when generating docs.  Even if not, there is no better
> >    place, is there?
> 
> No, that's the proper place, but make sure the docbook source is also
> picking up the .h file, I don't know if it currently does.
It does not, will fix in a v2.

Thanks
Uwe

[1] http://mid.gmane.org/1276846735-12105-2-git-send-email-u.kleine-koenig@pengutronix.de
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ACPI dock: move some functions to .init.text

[Uwe Kleine-König]

These functions are some of the few stoppers of moving
platform_device_register_data to .init.text, too.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/acpi/dock.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 3fe29e9..2b16563 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -929,7 +929,7 @@ static struct attribute_group dock_attribute_group = {
  * allocated and initialize a new dock station device.  Find all devices
  * that are on the dock station, and register for dock event notifications.
  */
-static int dock_add(acpi_handle handle)
+static int __init dock_add(acpi_handle handle)
 {
 	int ret, id;
 	struct dock_station ds, *dock_station;
@@ -1023,7 +1023,7 @@ static int dock_remove(struct dock_station *ds)
  *
  * This is called by acpi_walk_namespace to look for dock stations.
  */
-static acpi_status
+static __init acpi_status
 find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
 	if (is_dock(handle))
@@ -1032,7 +1032,7 @@ find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-static acpi_status
+static __init acpi_status
 find_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
 	/* If bay is a dock, it's already handled */
-- 
1.7.1

[PATCH 2/2 v2] Driver core: reduce duplicated code

[Uwe Kleine-König]

This makes the two similar functions platform_device_register_simple
and platform_device_register_data one line inline functions using a new
generic function platform_device_register_resndata.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

still unsolved is the naming issue, what do you think about
platform_device_register?

I marked the new function as __init_or_module in a separate patch to
make reverting it a bit easier, still I think it should be possible to
fix the caller if a problem occurs.

I changed the semantic slightly to only call
platform_device_add_resources if data != NULL instead of size != 0.  The
idea is to support wrappers like:

	#define add_blablub(id, pdata) \
		platform_device_register_resndata(NULL, "blablub", id, \
			NULL, 0, pdata, sizeof(struct blablub_platform_data))

that don't fail if pdata=NULL.  Ditto for res.

Best regards
Uwe

changed since v1:

 - fix docbook to pick up platform_device_register_simple and
   platform_device_register_data after moving them to
   <linux/platform_device.h>
 - only add_resources and add_data if res and data are non-NULL resp.

 Documentation/DocBook/device-drivers.tmpl |    1 +
 drivers/base/platform.c                   |  104 +++++++---------------------
 include/linux/platform_device.h           |   62 ++++++++++++++++-
 3 files changed, 85 insertions(+), 82 deletions(-)

diff --git a/Documentation/DocBook/device-drivers.tmpl b/Documentation/DocBook/device-drivers.tmpl
index 1b2dd4f..ecd35e9 100644
--- a/Documentation/DocBook/device-drivers.tmpl
+++ b/Documentation/DocBook/device-drivers.tmpl
@@ -111,6 +111,7 @@ X!Edrivers/base/attribute_container.c
 <!--
 X!Edrivers/base/interface.c
 -->
+!Iinclude/linux/platform_device.h
 !Edrivers/base/platform.c
 !Edrivers/base/bus.c
      </sect1>
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 26eb69d..ffcfd73 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -344,108 +344,56 @@ void platform_device_unregister(struct platform_device *pdev)
 EXPORT_SYMBOL_GPL(platform_device_unregister);
 
 /**
- * platform_device_register_simple - add a platform-level device and its resources
- * @name: base name of the device we're adding
- * @id: instance id
- * @res: set of resources that needs to be allocated for the device
- * @num: number of resources
- *
- * This function creates a simple platform device that requires minimal
- * resource and memory management. Canned release function freeing memory
- * allocated for the device allows drivers using such devices to be
- * unloaded without waiting for the last reference to the device to be
- * dropped.
+ * platform_device_register_resndata - add a platform-level device with
+ * resources and platform-specific data
  *
- * This interface is primarily intended for use with legacy drivers which
- * probe hardware directly.  Because such drivers create sysfs device nodes
- * themselves, rather than letting system infrastructure handle such device
- * enumeration tasks, they don't fully conform to the Linux driver model.
- * In particular, when such drivers are built as modules, they can't be
- * "hotplugged".
- *
- * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
- */
-struct platform_device *platform_device_register_simple(const char *name,
-							int id,
-							const struct resource *res,
-							unsigned int num)
-{
-	struct platform_device *pdev;
-	int retval;
-
-	pdev = platform_device_alloc(name, id);
-	if (!pdev) {
-		retval = -ENOMEM;
-		goto error;
-	}
-
-	if (num) {
-		retval = platform_device_add_resources(pdev, res, num);
-		if (retval)
-			goto error;
-	}
-
-	retval = platform_device_add(pdev);
-	if (retval)
-		goto error;
-
-	return pdev;
-
-error:
-	platform_device_put(pdev);
-	return ERR_PTR(retval);
-}
-EXPORT_SYMBOL_GPL(platform_device_register_simple);
-
-/**
- * platform_device_register_data - add a platform-level device with platform-specific data
  * @parent: parent device for the device we're adding
  * @name: base name of the device we're adding
  * @id: instance id
+ * @res: set of resources that needs to be allocated for the device
+ * @num: number of resources
  * @data: platform specific data for this platform device
  * @size: size of platform specific data
  *
- * This function creates a simple platform device that requires minimal
- * resource and memory management. Canned release function freeing memory
- * allocated for the device allows drivers using such devices to be
- * unloaded without waiting for the last reference to the device to be
- * dropped.
- *
  * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
  */
-struct platform_device *platform_device_register_data(
+struct platform_device *platform_device_register_resndata(
 		struct device *parent,
 		const char *name, int id,
+		const struct resource *res, unsigned int num,
 		const void *data, size_t size)
 {
+	int ret = -ENOMEM;
 	struct platform_device *pdev;
-	int retval;
 
 	pdev = platform_device_alloc(name, id);
-	if (!pdev) {
-		retval = -ENOMEM;
-		goto error;
-	}
+	if (!pdev)
+		goto err;
 
 	pdev->dev.parent = parent;
 
-	if (size) {
-		retval = platform_device_add_data(pdev, data, size);
-		if (retval)
-			goto error;
+	if (res) {
+		ret = platform_device_add_resources(pdev, res, num);
+		if (ret)
+			goto err;
 	}
 
-	retval = platform_device_add(pdev);
-	if (retval)
-		goto error;
+	if (data) {
+		ret = platform_device_add_data(pdev, data, size);
+		if (ret)
+			goto err;
+	}
 
-	return pdev;
+	ret = platform_device_add(pdev);
+	if (ret) {
+err:
+		platform_device_put(pdev);
+		return ERR_PTR(ret);
+	}
 
-error:
-	platform_device_put(pdev);
-	return ERR_PTR(retval);
+	return pdev;
 }
-EXPORT_SYMBOL_GPL(platform_device_register_data);
+EXPORT_SYMBOL_GPL(platform_device_register_resndata);
 
 static int platform_drv_probe(struct device *_dev)
 {
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 5417944..d7ecad0 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -43,10 +43,64 @@ extern struct resource *platform_get_resource_byname(struct platform_device *, u
 extern int platform_get_irq_byname(struct platform_device *, const char *);
 extern int platform_add_devices(struct platform_device **, int);
 
-extern struct platform_device *platform_device_register_simple(const char *, int id,
-					const struct resource *, unsigned int);
-extern struct platform_device *platform_device_register_data(struct device *,
-		const char *, int, const void *, size_t);
+extern struct platform_device *platform_device_register_resndata(
+		struct device *parent, const char *name, int id,
+		const struct resource *res, unsigned int num,
+		const void *data, size_t size);
+
+/**
+ * platform_device_register_simple - add a platform-level device and its resources
+ * @name: base name of the device we're adding
+ * @id: instance id
+ * @res: set of resources that needs to be allocated for the device
+ * @num: number of resources
+ *
+ * This function creates a simple platform device that requires minimal
+ * resource and memory management. Canned release function freeing memory
+ * allocated for the device allows drivers using such devices to be
+ * unloaded without waiting for the last reference to the device to be
+ * dropped.
+ *
+ * This interface is primarily intended for use with legacy drivers which
+ * probe hardware directly.  Because such drivers create sysfs device nodes
+ * themselves, rather than letting system infrastructure handle such device
+ * enumeration tasks, they don't fully conform to the Linux driver model.
+ * In particular, when such drivers are built as modules, they can't be
+ * "hotplugged".
+ *
+ * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
+ */
+static inline struct platform_device *platform_device_register_simple(
+		const char *name, int id,
+		const struct resource *res, unsigned int num)
+{
+	return platform_device_register_resndata(NULL, name, id,
+			res, num, NULL, 0);
+}
+
+/**
+ * platform_device_register_data - add a platform-level device with platform-specific data
+ * @parent: parent device for the device we're adding
+ * @name: base name of the device we're adding
+ * @id: instance id
+ * @data: platform specific data for this platform device
+ * @size: size of platform specific data
+ *
+ * This function creates a simple platform device that requires minimal
+ * resource and memory management. Canned release function freeing memory
+ * allocated for the device allows drivers using such devices to be
+ * unloaded without waiting for the last reference to the device to be
+ * dropped.
+ *
+ * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
+ */
+static inline struct platform_device *platform_device_register_data(
+		struct device *parent, const char *name, int id,
+		const void *data, size_t size)
+{
+	return platform_device_register_resndata(parent, name, id,
+			NULL, 0, data, size);
+}
 
 extern struct platform_device *platform_device_alloc(const char *name, int id);
 extern int platform_device_add_resources(struct platform_device *pdev,
-- 
1.7.1

[PATCH 3/2] Driver core: move platform device creation helpers to .init.text (if MODULE=n)

[Uwe Kleine-König]

Platform devices should only be called by init code, so it should be
possible to move creation helpers to .init.text -- at least if modules
are disabled.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

for a x64_64_defconfig with CONFIG_MODULES disabled this introduces two new
section mismatches.  I already sent patches for those[1].

Best regards
Uwe


[1] one in this thread and the other can be found at

	http://thread.gmane.org/gmane.linux.kernel.wireless.general/52404/focus=52405

 drivers/base/platform.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index ffcfd73..1bb8faa 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -357,7 +357,7 @@ EXPORT_SYMBOL_GPL(platform_device_unregister);
  *
  * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
  */
-struct platform_device *platform_device_register_resndata(
+struct platform_device *__init_or_module platform_device_register_resndata(
 		struct device *parent,
 		const char *name, int id,
 		const struct resource *res, unsigned int num,
-- 
1.7.1

Re: [PATCH 2/2 v2] Driver core: reduce duplicated code

[Greg KH]

On Mon, Jun 21, 2010 at 04:11:44PM +0200, Uwe Kleine-König wrote:
> This makes the two similar functions platform_device_register_simple
> and platform_device_register_data one line inline functions using a new
> generic function platform_device_register_resndata.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> still unsolved is the naming issue, what do you think about
> platform_device_register?

We already have a platform_device_register() function :)

> I marked the new function as __init_or_module in a separate patch to
> make reverting it a bit easier, still I think it should be possible to
> fix the caller if a problem occurs.
> 
> I changed the semantic slightly to only call
> platform_device_add_resources if data != NULL instead of size != 0.  The
> idea is to support wrappers like:
> 
> 	#define add_blablub(id, pdata) \
> 		platform_device_register_resndata(NULL, "blablub", id, \
> 			NULL, 0, pdata, sizeof(struct blablub_platform_data))
> 
> that don't fail if pdata=NULL.  Ditto for res.

That's fine, but why would you want to have a #define for something like
this?  Is it really needed?

Anyway, this version looks fine to me, I'll go apply it.

thanks,

greg k-h

Re: [PATCH 2/2 v2] Driver core: reduce duplicated code

[Uwe Kleine-König]

Hi Greg,

> > I changed the semantic slightly to only call
> > platform_device_add_resources if data != NULL instead of size != 0.  The
> > idea is to support wrappers like:
> > 
> > 	#define add_blablub(id, pdata) \
> > 		platform_device_register_resndata(NULL, "blablub", id, \
> > 			NULL, 0, pdata, sizeof(struct blablub_platform_data))
> > 
> > that don't fail if pdata=NULL.  Ditto for res.
> 
> That's fine, but why would you want to have a #define for something like
> this?  Is it really needed?
Well, what is really needed?  I intend to use it on arm/imx.  I have
several different machines using similar SoCs and so I want to have a
function à la:

	struct platform_device *__init imx_add_imx_i2c(int id,
		resource_size_t iobase, resource_size_t iosize, int irq,
		const struct imxi2c_platform_data *pdata)

that builds a struct resource[] and then calls
platform_device_register_resndata().  And then I have a set of macros
like:

	#define imx21_add_i2c_imx(pdata)	\
		imx_add_imx_i2c(0, MX2x_I2C_BASE_ADDR, SZ_4K, MX2x_INT_I2C, pdata)
	#define imx25_add_imx_i2c0(pdata)	\
		imx_add_imx_i2c(0, MX25_I2C1_BASE_ADDR, SZ_16K, MX25_INT_I2C1, pdata)
	##define imx25_add_imx_i2c1(pdata)	\
		imx_add_imx_i2c(1, MX25_I2C2_BASE_ADDR, SZ_16K, MX25_INT_I2C2, pdata)

etc.  The final goal is to get rid of files like
arch/arm/mach-mx3/devices.c.

> Anyway, this version looks fine to me, I'll go apply it.
\o/

Best regards and thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2 v2] Driver core: reduce duplicated code

[Eric Miao]

2010/6/22 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hi Greg,
>
>> > I changed the semantic slightly to only call
>> > platform_device_add_resources if data != NULL instead of size != 0.  The
>> > idea is to support wrappers like:
>> >
>> >     #define add_blablub(id, pdata) \
>> >             platform_device_register_resndata(NULL, "blablub", id, \
>> >                     NULL, 0, pdata, sizeof(struct blablub_platform_data))
>> >
>> > that don't fail if pdata=NULL.  Ditto for res.
>>
>> That's fine, but why would you want to have a #define for something like
>> this?  Is it really needed?
> Well, what is really needed?  I intend to use it on arm/imx.  I have
> several different machines using similar SoCs and so I want to have a
> function à la:
>
>        struct platform_device *__init imx_add_imx_i2c(int id,
>                resource_size_t iobase, resource_size_t iosize, int irq,
>                const struct imxi2c_platform_data *pdata)
>
> that builds a struct resource[] and then calls
> platform_device_register_resndata().  And then I have a set of macros
> like:
>
>        #define imx21_add_i2c_imx(pdata)        \
>                imx_add_imx_i2c(0, MX2x_I2C_BASE_ADDR, SZ_4K, MX2x_INT_I2C, pdata)
>        #define imx25_add_imx_i2c0(pdata)       \
>                imx_add_imx_i2c(0, MX25_I2C1_BASE_ADDR, SZ_16K, MX25_INT_I2C1, pdata)
>        ##define imx25_add_imx_i2c1(pdata)      \
>                imx_add_imx_i2c(1, MX25_I2C2_BASE_ADDR, SZ_16K, MX25_INT_I2C2, pdata)
>
> etc.  The final goal is to get rid of files like
> arch/arm/mach-mx3/devices.c.
>

Hi Uwe,

I suggest you to have a look into arch/arm/mach-mmp/devices.c and
arch/arm/mach-mmp/pxa{168,910}.c as well as
arch/arm/mach-mmp/include/mach/pxa{168,910}.h, maybe we can find
some common practice.

>> Anyway, this version looks fine to me, I'll go apply it.
> \o/
>
> Best regards and thanks
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>

Re: [PATCH 2/2 v2] Driver core: reduce duplicated code

[Uwe Kleine-König]

Hi Eric,

On Mon, Jun 28, 2010 at 12:55:45PM +0800, Eric Miao wrote:
> I suggest you to have a look into arch/arm/mach-mmp/devices.c and
> arch/arm/mach-mmp/pxa{168,910}.c as well as
> arch/arm/mach-mmp/include/mach/pxa{168,910}.h, maybe we can find
> some common practice.
I think I like this approach in general, I already thought about not
passing all parameters as function/macro arguments, too.  But maybe this
becomes too excessive for imx as I would need too many of these device
desc for the different imx variants?!

Anyhow a few things I thought when looking in the files you suggested:

 - Why not use an array for all uart devdescs, maybe the code for
   pxa168_add_uart could become a bit smaller then?:

	extern struct pxa_device_desc pxa168_device_uart[2];
	...
	static inline int pxa168_add_uart(int id)
	{
		struct pxa_device_desc *d = pxa168_device_uart + id;

		if (id < 0 || id > 2)
			return -EINVAL;

		return pxa_register_device(d, NULL, 0);
	}

   (Ditto for the other types obviously.)

 - shouldn't all these pxa_device_descs and pxa168_add_$device functions
   be __initdata and __init?

 - pxa_register_device is better than my add_resndata function in (at
   least) one aspect as it sets coherent_dma_mask, too.  This is
   something I missed when trying to add mxc-mmc (IIRC) devices.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2 v2] Driver core: reduce duplicated code

[Eric Miao]

2010/6/28 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hi Eric,
>
> On Mon, Jun 28, 2010 at 12:55:45PM +0800, Eric Miao wrote:
>> I suggest you to have a look into arch/arm/mach-mmp/devices.c and
>> arch/arm/mach-mmp/pxa{168,910}.c as well as
>> arch/arm/mach-mmp/include/mach/pxa{168,910}.h, maybe we can find
>> some common practice.
> I think I like this approach in general, I already thought about not
> passing all parameters as function/macro arguments, too.  But maybe this
> becomes too excessive for imx as I would need too many of these device
> desc for the different imx variants?!
>
> Anyhow a few things I thought when looking in the files you suggested:
>
>  - Why not use an array for all uart devdescs, maybe the code for
>   pxa168_add_uart could become a bit smaller then?:
>
>        extern struct pxa_device_desc pxa168_device_uart[2];
>        ...
>        static inline int pxa168_add_uart(int id)
>        {
>                struct pxa_device_desc *d = pxa168_device_uart + id;
>
>                if (id < 0 || id > 2)
>                        return -EINVAL;
>
>                return pxa_register_device(d, NULL, 0);
>        }
>
>   (Ditto for the other types obviously.)

That's a good suggestion, yet it came that way for two reasons:

1. the initial naming mess, uart0 was later renamed to uart1, e.g.
2. and the restrictions of PXA{168,910}_DEVICE() macros, these
   macros are handy to simplify the definition, but may require fancy
   tricks to make it support array

>
>  - shouldn't all these pxa_device_descs and pxa168_add_$device functions
>   be __initdata and __init?
>

pxa{168,910}_add_device() are actually 'static inline' so my assumption
is they will be inlined when referenced, otherwise won't occupy any code
space. The *_descs, however, they are __initdata if you look into the
definitions of PXA{168,910}_DEVICES

>  - pxa_register_device is better than my add_resndata function in (at
>   least) one aspect as it sets coherent_dma_mask, too.  This is
>   something I missed when trying to add mxc-mmc (IIRC) devices.
>
> Thanks
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>

[PATCH RESEND] ACPI dock: move some functions to .init.text

[Uwe Kleine-König]

find_dock and find_bay are only called by dock_init which lives in
.init.text dock_add is only called by find_dock and find_bay.  So all
three functions can be moved to .init.text, too.

This fixes:

        WARNING: vmlinux.o(.text+0x2134b7): Section mismatch in reference from the function dock_add() to the function .init.text:platform_device_register_resndata()
        The function dock_add() references
        the function __init platform_device_register_resndata().
        This is often because dock_add lacks a __init
        annotation or the annotation of platform_device_register_resndata is wrong.

for a build with unset CONFIG_MODULES.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

since the first submission of the patch back in June I reworded the
commit log to make it (hopefully) more understandable.  Since then
platform_device_register_data was marked __init_or_module, so the
problem is real now (and only occurs with CONFIG_MODULES=n).

Best regards
Uwe

 drivers/acpi/dock.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index 3fe29e9..2b16563 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -929,7 +929,7 @@ static struct attribute_group dock_attribute_group = {
  * allocated and initialize a new dock station device.  Find all devices
  * that are on the dock station, and register for dock event notifications.
  */
-static int dock_add(acpi_handle handle)
+static int __init dock_add(acpi_handle handle)
 {
 	int ret, id;
 	struct dock_station ds, *dock_station;
@@ -1023,7 +1023,7 @@ static int dock_remove(struct dock_station *ds)
  *
  * This is called by acpi_walk_namespace to look for dock stations.
  */
-static acpi_status
+static __init acpi_status
 find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
 	if (is_dock(handle))
@@ -1032,7 +1032,7 @@ find_dock(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-static acpi_status
+static __init acpi_status
 find_bay(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
 	/* If bay is a dock, it's already handled */
-- 
1.7.2.3

Re: [PATCH RESEND] ACPI dock: move some functions to .init.text

[Greg KH]

On Tue, Oct 19, 2010 at 09:13:39AM +0200, Uwe Kleine-König wrote:
> find_dock and find_bay are only called by dock_init which lives in
> .init.text dock_add is only called by find_dock and find_bay.  So all
> three functions can be moved to .init.text, too.
> 
> This fixes:
> 
>         WARNING: vmlinux.o(.text+0x2134b7): Section mismatch in reference from the function dock_add() to the function .init.text:platform_device_register_resndata()
>         The function dock_add() references
>         the function __init platform_device_register_resndata().
>         This is often because dock_add lacks a __init
>         annotation or the annotation of platform_device_register_resndata is wrong.
> 
> for a build with unset CONFIG_MODULES.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Re: [PATCH RESEND] ACPI dock: move some functions to .init.text

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center