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

[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 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

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/  |
>