[PATCH] platform: add new device registration helper

[PATCH] platform: add new device registration helper

[Dmitry Baryshkov]

Add a helper that registers simple platform_device
w/o resources but with parent and device data.

This is usefull to cleanup platform code from code that
registers such simple devices as leds-gpio, generic-bl,
etc.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 drivers/base/platform.c         |   47 +++++++++++++++++++++++++++++++++++++++
 include/linux/platform_device.h |    2 +
 2 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3f94039..fcd9f97 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -391,6 +391,53 @@ error:
 }
 EXPORT_SYMBOL_GPL(platform_device_register_simple);
 
+/**
+ * platform_device_register_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.
+ */
+struct platform_device *platform_device_register_data(
+		struct device *parent,
+		const char *name, int id,
+		const void *data, size_t size)
+{
+	struct platform_device *pdev;
+	int retval;
+
+	pdev = platform_device_alloc(name, id);
+	if (!pdev) {
+		retval = -ENOMEM;
+		goto error;
+	}
+
+	pdev->dev.parent = parent;
+
+	if (size) {
+		retval = platform_device_add_data(pdev, data, size);
+		if (retval)
+			goto error;
+	}
+
+	retval = platform_device_add(pdev);
+	if (retval)
+		goto error;
+
+	return pdev;
+
+error:
+	platform_device_put(pdev);
+	return ERR_PTR(retval);
+}
+
 static int platform_drv_probe(struct device *_dev)
 {
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 95ac21a..4b8cc6a 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -37,6 +37,8 @@ extern int platform_add_devices(struct platform_device **, int);
 
 extern struct platform_device *platform_device_register_simple(const char *, int id,
 					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_alloc(const char *name, int id);
 extern int platform_device_add_resources(struct platform_device *pdev, struct resource *res, unsigned int num);
-- 
1.5.6.5

Re: [PATCH] platform: add new device registration helper

[Andrew Morton]

On Fri,  5 Sep 2008 01:30:16 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> Add a helper that registers simple platform_device
> w/o resources but with parent and device data.
> 
> This is usefull to cleanup platform code from code that
> registers such simple devices as leds-gpio, generic-bl,
> etc.
> 

nits:

>  include/linux/platform_device.h |    2 +
>  2 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3f94039..fcd9f97 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -391,6 +391,53 @@ error:
>  }
>  EXPORT_SYMBOL_GPL(platform_device_register_simple);
>  
> +/**
> + * platform_device_register_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.

Should document the return value (tested with IS_ERR)

> + */
> +struct platform_device *platform_device_register_data(
> +		struct device *parent,
> +		const char *name, int id,
> +		const void *data, size_t size)

Something like

struct platform_device *platform_device_register_data(struct device *parent,
			const char *name, int id, const void *data, size_t size)

would be more typical.  

> +{
> +	struct platform_device *pdev;
> +	int retval;
> +
> +	pdev = platform_device_alloc(name, id);
> +	if (!pdev) {
> +		retval = -ENOMEM;
> +		goto error;
> +	}
> +
> +	pdev->dev.parent = parent;
> +
> +	if (size) {
> +		retval = platform_device_add_data(pdev, data, size);
> +		if (retval)
> +			goto error;
> +	}
> +
> +	retval = platform_device_add(pdev);
> +	if (retval)
> +		goto error;
> +
> +	return pdev;
> +
> +error:
> +	platform_device_put(pdev);

Are you sure this can't trigger a !kobj->state_initialized warning in
kobject_put()?

> +	return ERR_PTR(retval);
> +}
> +
>
> ...
>
> +extern struct platform_device *platform_device_register_data(struct device *,
> +		const char *, int, const void *, size_t);

It's nice (IMO) to include the names of the args, for documentation
purposes.  Obviously the surrounding code didn't agree.

Re: [PATCH] platform: add new device registration helper

[Andrew Morton]

also,

On Fri,  5 Sep 2008 01:30:16 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

>  }
>  EXPORT_SYMBOL_GPL(platform_device_register_simple);
>  
> ...
>
> +struct platform_device *platform_device_register_data(

When you start implementing callers, please check whether
platform_device_register_data() should have been exported to modules.

Re: [PATCH] platform: add new device registration helper

[Dmitry Baryshkov]

On Thu, Sep 04, 2008 at 05:20:50PM -0700, Andrew Morton wrote:
> On Fri,  5 Sep 2008 01:30:16 +0400
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> 
> > Add a helper that registers simple platform_device
> > w/o resources but with parent and device data.
> > 
> > This is usefull to cleanup platform code from code that
> > registers such simple devices as leds-gpio, generic-bl,
> > etc.
> > 
> 
> nits:
> 
> > + *
> > + * 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.
> 
> Should document the return value (tested with IS_ERR)

Will the wording "Returns a valid platform device, or a valid IS_ERR
condition containing errno" be appropriate?

> > +{
> > +	struct platform_device *pdev;
> > +	int retval;
> > +
> > +	pdev = platform_device_alloc(name, id);
> > +	if (!pdev) {
> > +		retval = -ENOMEM;
> > +		goto error;
> > +	}
> > +
> > +	pdev->dev.parent = parent;
> > +
> > +	if (size) {
> > +		retval = platform_device_add_data(pdev, data, size);
> > +		if (retval)
> > +			goto error;
> > +	}
> > +
> > +	retval = platform_device_add(pdev);
> > +	if (retval)
> > +		goto error;
> > +
> > +	return pdev;
> > +
> > +error:
> > +	platform_device_put(pdev);
> 
> Are you sure this can't trigger a !kobj->state_initialized warning in
> kobject_put()?

No. The kobject is initialised in platform_device_alloc() ->
device_initialise()

> >
> > ...
> >
> > +extern struct platform_device *platform_device_register_data(struct device *,
> > +		const char *, int, const void *, size_t);
> 
> It's nice (IMO) to include the names of the args, for documentation
> purposes.  Obviously the surrounding code didn't agree.

I just copied the surrounding code style. If you wish, I can put the
names for this function back.

BTW: would you prefer the followup "fix" patch or just the replacement
one?


-- 
With best wishes
Dmitry

Re: [PATCH] platform: add new device registration helper

[Andrew Morton]

On Fri, 5 Sep 2008 14:33:18 +0400 Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> On Thu, Sep 04, 2008 at 05:20:50PM -0700, Andrew Morton wrote:
> > On Fri,  5 Sep 2008 01:30:16 +0400
> > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> > 
> > > Add a helper that registers simple platform_device
> > > w/o resources but with parent and device data.
> > > 
> > > This is usefull to cleanup platform code from code that
> > > registers such simple devices as leds-gpio, generic-bl,
> > > etc.
> > > 
> > 
> > nits:
> > 
> > > + *
> > > + * 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.
> > 
> > Should document the return value (tested with IS_ERR)
> 
> Will the wording "Returns a valid platform device, or a valid IS_ERR
> condition containing errno" be appropriate?

Sounds good.

> 
> BTW: would you prefer the followup "fix" patch or just the replacement
> one?
> 

Doesn't worry me at this stage, but Greg would probably like a whole
new patch, as he probably didn't merge version 1.