[PATCH] Minor change to platform_device_register_simple prototype

[PATCH] Minor change to platform_device_register_simple prototype

[Jean Delvare]

Hi all,

Propose patch for platform driver core. Greg, can you add it to your
driver queue?

Thanks.

Content-Disposition: inline; filename=driver-platform-device-name-as-const-char.patch

The name parameter of platform_device_register_simple should be of
type const char * instead of char *, as we simply pass it to
platform_device_alloc, where it has type const char *.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/base/platform.c         |    2 +-
 include/linux/platform_device.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.15-rc2.orig/drivers/base/platform.c	2005-11-13 21:02:31.000000000 +0100
+++ linux-2.6.15-rc2/drivers/base/platform.c	2005-12-05 20:44:43.000000000 +0100
@@ -327,7 +327,7 @@
  *	to be unloaded iwithout waiting for the last reference to the device
  *	to be dropped.
  */
-struct platform_device *platform_device_register_simple(char *name, unsigned int id,
+struct platform_device *platform_device_register_simple(const char *name, unsigned int id,
 							struct resource *res, unsigned int num)
 {
 	struct platform_device *pdev;
--- linux-2.6.15-rc2.orig/include/linux/platform_device.h	2005-11-13 21:02:59.000000000 +0100
+++ linux-2.6.15-rc2/include/linux/platform_device.h	2005-12-05 20:44:30.000000000 +0100
@@ -35,7 +35,7 @@
 extern int platform_get_irq_byname(struct platform_device *, char *);
 extern int platform_add_devices(struct platform_device **, int);
 
-extern struct platform_device *platform_device_register_simple(char *, unsigned int, struct resource *, unsigned int);
+extern struct platform_device *platform_device_register_simple(const char *, unsigned int, struct resource *, unsigned int);
 
 extern struct platform_device *platform_device_alloc(const char *name, unsigned int id);
 extern int platform_device_add_resources(struct platform_device *pdev, struct resource *res, unsigned int num);


-- 
Jean Delvare

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> The name parameter of platform_device_register_simple should be of
> type const char * instead of char *, as we simply pass it to
> platform_device_alloc, where it has type const char *.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

However, I've been wondering whether we want to keep this "simple"
interface around long-term given that we now have a more flexible
platform device allocation interface - I don't particularly like
having superfluous interfaces for folk to get confused with.

---
 drivers/base/platform.c         |    2 +-
 include/linux/platform_device.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.15-rc2.orig/drivers/base/platform.c	2005-11-13 21:02:31.000000000 +0100
+++ linux-2.6.15-rc2/drivers/base/platform.c	2005-12-05 20:44:43.000000000 +0100
@@ -327,7 +327,7 @@
  *	to be unloaded iwithout waiting for the last reference to the device
  *	to be dropped.
  */
-struct platform_device *platform_device_register_simple(char *name, unsigned int id,
+struct platform_device *platform_device_register_simple(const char *name, unsigned int id,
 							struct resource *res, unsigned int num)
 {
 	struct platform_device *pdev;
--- linux-2.6.15-rc2.orig/include/linux/platform_device.h	2005-11-13 21:02:59.000000000 +0100
+++ linux-2.6.15-rc2/include/linux/platform_device.h	2005-12-05 20:44:30.000000000 +0100
@@ -35,7 +35,7 @@
 extern int platform_get_irq_byname(struct platform_device *, char *);
 extern int platform_add_devices(struct platform_device **, int);
 
-extern struct platform_device *platform_device_register_simple(char *, unsigned int, struct resource *, unsigned int);
+extern struct platform_device *platform_device_register_simple(const char *, unsigned int, struct resource *, unsigned int);
 
 extern struct platform_device *platform_device_alloc(const char *name, unsigned int id);
 extern int platform_device_add_resources(struct platform_device *pdev, struct resource *res, unsigned int num);

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On Monday 05 December 2005 15:27, Russell King wrote:
> On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> > The name parameter of platform_device_register_simple should be of
> > type const char * instead of char *, as we simply pass it to
> > platform_device_alloc, where it has type const char *.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> 
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> However, I've been wondering whether we want to keep this "simple"
> interface around long-term given that we now have a more flexible
> platform device allocation interface - I don't particularly like
> having superfluous interfaces for folk to get confused with.

Now that you made platform_device_alloc install default release
handler there is no need to have the _simple interface. I will
convert input devices (main users of _simple) to the new interface
and then we can get rid of it.

-- 
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Greg KH]

On Wed, Dec 07, 2005 at 01:05:39AM -0500, Dmitry Torokhov wrote:
> On Monday 05 December 2005 15:27, Russell King wrote:
> > On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> > > The name parameter of platform_device_register_simple should be of
> > > type const char * instead of char *, as we simply pass it to
> > > platform_device_alloc, where it has type const char *.
> > > 
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > 
> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > 
> > However, I've been wondering whether we want to keep this "simple"
> > interface around long-term given that we now have a more flexible
> > platform device allocation interface - I don't particularly like
> > having superfluous interfaces for folk to get confused with.
> 
> Now that you made platform_device_alloc install default release
> handler there is no need to have the _simple interface. I will
> convert input devices (main users of _simple) to the new interface
> and then we can get rid of it.

That sounds like a very good idea.

thanks,

greg k-h

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/7/05, Greg KH <greg@kroah.com> wrote:
> On Wed, Dec 07, 2005 at 01:05:39AM -0500, Dmitry Torokhov wrote:
> > On Monday 05 December 2005 15:27, Russell King wrote:
> > > On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> > > > The name parameter of platform_device_register_simple should be of
> > > > type const char * instead of char *, as we simply pass it to
> > > > platform_device_alloc, where it has type const char *.
> > > >
> > > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > >
> > > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > >
> > > However, I've been wondering whether we want to keep this "simple"
> > > interface around long-term given that we now have a more flexible
> > > platform device allocation interface - I don't particularly like
> > > having superfluous interfaces for folk to get confused with.
> >
> > Now that you made platform_device_alloc install default release
> > handler there is no need to have the _simple interface. I will
> > convert input devices (main users of _simple) to the new interface
> > and then we can get rid of it.
>
> That sounds like a very good idea.
>

Another thing - bunch of input code currently creates platform devices
but does not create corresponding platform drivers (because they don't
support suspend/resume or shutdown and probing is done right there in
module init function).

What is the genral policy on platform devices? Should they always have
a corresponding driver ir it is OK to leave them without one?

--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Jean Delvare]

Hi Dmitry,

> Another thing - bunch of input code currently creates platform devices
> but does not create corresponding platform drivers (because they don't
> support suspend/resume or shutdown and probing is done right there in
> module init function).
> 
> What is the general policy on platform devices? Should they always have
> a corresponding driver or is it OK to leave them without one?

If it wasn't OK, I'd expect platform_device_alloc and
platform_device_register to fail when no matching driver is found.
Since they do not, I'd guess it is considered OK not to have a matching
driver. But that's really only a guess and not a replacement for
Russell's (or Greg's) authoritative answer.

Reciprocally, if it is finally decided that it is *not* OK to have a
platform device without a driver, they we want to make both functions
mentioned above fail when no match is found.

I am interested in the answer myself, as I am just realizing that my
own driver registers a platform driver but doesn't use it at all, just
like Dmitry described for his input drivers - so if I am allowed not to
register this platform driver I may just drop that part.

Thanks,
-- 
Jean Delvare

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/8/05, Jean Delvare <khali@linux-fr.org> wrote:
> > Another thing - bunch of input code currently creates platform devices
> > but does not create corresponding platform drivers (because they don't
> > support suspend/resume or shutdown and probing is done right there in
> > module init function).
> >
> > What is the general policy on platform devices? Should they always have
> > a corresponding driver or is it OK to leave them without one?
>
> If it wasn't OK, I'd expect platform_device_alloc and
> platform_device_register to fail when no matching driver is found.
> Since they do not, I'd guess it is considered OK not to have a matching
> driver. But that's really only a guess and not a replacement for
> Russell's (or Greg's) authoritative answer.
>
> Reciprocally, if it is finally decided that it is *not* OK to have a
> platform device without a driver, they we want to make both functions
> mentioned above fail when no match is found.
>

I don't think so - some platforms could discover platform devices
separately from drivers being loaded (think separate modules). Then,
like with PCI, they would have devices without drivers... My question
was more along the loines "do we want to waste some memory registering
a driver that does absolutely nothing except for signalling userspace
that drive is indeed has a driver attached". And letting userspace
know that device is handled is probably a good thing.

--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Thu, Dec 08, 2005 at 10:37:05PM +0100, Jean Delvare wrote:
> Hi Dmitry,
> 
> > Another thing - bunch of input code currently creates platform devices
> > but does not create corresponding platform drivers (because they don't
> > support suspend/resume or shutdown and probing is done right there in
> > module init function).
> > 
> > What is the general policy on platform devices? Should they always have
> > a corresponding driver or is it OK to leave them without one?
> 
> If it wasn't OK, I'd expect platform_device_alloc and
> platform_device_register to fail when no matching driver is found.

You're actually talking about driver model convention, which is that
if a driver for a device is missing, we do not return an error - a
hotplug event (or whatever is the flavour of the month) might provide
a driver.

For example, you might have a SMC91x device on your board, and you
may have chosen to build the driver as a module.  You wouldn't want
the device to not register.

Why should a driver registering its own platform device be treated
any different (from any platform provided device or indeed the rest
of the device/driver model)?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/7/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> On Monday 05 December 2005 15:27, Russell King wrote:
> > On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> > > The name parameter of platform_device_register_simple should be of
> > > type const char * instead of char *, as we simply pass it to
> > > platform_device_alloc, where it has type const char *.
> > >
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> >
> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> >
> > However, I've been wondering whether we want to keep this "simple"
> > interface around long-term given that we now have a more flexible
> > platform device allocation interface - I don't particularly like
> > having superfluous interfaces for folk to get confused with.
>
> Now that you made platform_device_alloc install default release
> handler there is no need to have the _simple interface. I will
> convert input devices (main users of _simple) to the new interface
> and then we can get rid of it.
>

I have started moving drivers from the "_simple" interface and I found
that I'm missing platform_device_del that would complement
platform_device_add. Would you object to having such a function, like
we do for other sysfs objects? With it one can write somthing like
this:

 err_delete_device:
        platform_device_del(i8042_platform_device);
 err_free_device:
        platform_device_put(i8042_platform_device);
 err_unregister_driver:
        platform_driver_unregister(&i8042_driver);


--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov wrote:
> On 12/7/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > On Monday 05 December 2005 15:27, Russell King wrote:
> > > On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> > > > The name parameter of platform_device_register_simple should be of
> > > > type const char * instead of char *, as we simply pass it to
> > > > platform_device_alloc, where it has type const char *.
> > > >
> > > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > >
> > > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > >
> > > However, I've been wondering whether we want to keep this "simple"
> > > interface around long-term given that we now have a more flexible
> > > platform device allocation interface - I don't particularly like
> > > having superfluous interfaces for folk to get confused with.
> >
> > Now that you made platform_device_alloc install default release
> > handler there is no need to have the _simple interface. I will
> > convert input devices (main users of _simple) to the new interface
> > and then we can get rid of it.
> >
> 
> I have started moving drivers from the "_simple" interface and I found
> that I'm missing platform_device_del that would complement
> platform_device_add. Would you object to having such a function, like
> we do for other sysfs objects? With it one can write somthing like
> this:

Greg and myself discussed that, and we decided that it was adding
unnecessary complexity to the interface.  Maybe Greg's view has
changed?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found
> > that I'm missing platform_device_del that would complement
> > platform_device_add. Would you object to having such a function, like
> > we do for other sysfs objects? With it one can write somthing like
> > this:
>
> Greg and myself discussed that, and we decided that it was adding
> unnecessary complexity to the interface.  Maybe Greg's view has
> changed?
>

How do you write error handling path without the _del function if
platform_device_add is not the last call? you can't call
platform_device_unregister() and then platform_device_put(). And I
don't like to take extra references in error path or assign the
pointer to NULL in teh middle of unwinding...

--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Wed, Dec 07, 2005 at 01:23:11PM -0500, Dmitry Torokhov wrote:
> On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found
> > > that I'm missing platform_device_del that would complement
> > > platform_device_add. Would you object to having such a function, like
> > > we do for other sysfs objects? With it one can write somthing like
> > > this:
> >
> > Greg and myself discussed that, and we decided that it was adding
> > unnecessary complexity to the interface.  Maybe Greg's view has
> > changed?
> >
> 
> How do you write error handling path without the _del function if
> platform_device_add is not the last call? you can't call
> platform_device_unregister() and then platform_device_put(). And I
> don't like to take extra references in error path or assign the
> pointer to NULL in teh middle of unwinding...

The example code in the commit comments contains a complete example of
registering a platform device, and cleaning up should something go
wrong with that process.

Unregistering is just a matter of calling platform_device_unregister().
An unregister call is a del + put in exactly the same way as it is
throughout the rest of the driver model.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> On Wed, Dec 07, 2005 at 01:23:11PM -0500, Dmitry Torokhov wrote:
> > On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > > On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found
> > > > that I'm missing platform_device_del that would complement
> > > > platform_device_add. Would you object to having such a function, like
> > > > we do for other sysfs objects? With it one can write somthing like
> > > > this:
> > >
> > > Greg and myself discussed that, and we decided that it was adding
> > > unnecessary complexity to the interface.  Maybe Greg's view has
> > > changed?
> > >
> >
> > How do you write error handling path without the _del function if
> > platform_device_add is not the last call? you can't call
> > platform_device_unregister() and then platform_device_put(). And I
> > don't like to take extra references in error path or assign the
> > pointer to NULL in teh middle of unwinding...
>
> The example code in the commit comments contains a complete example of
> registering a platform device, and cleaning up should something go
> wrong with that process.
>

The problem with what you proposing is that one will have to code 2
cleanup code paths - one when platform_device_add fails (in this case
you just call platform_device_put) and another one when
platfrom_device_add succeeds but something else fails. In the second
case you have to use platfrom_device_unregister to release resources
but can't use platform_device_put because the device will most likely
be released by plaform_device_unregister. I prefer having single
cleanup code path, like most other drivers have.

> Unregistering is just a matter of calling platform_device_unregister().
> An unregister call is a del + put in exactly the same way as it is
> throughout the rest of the driver model.
>

Yes, and it works just fine everywhere except in initialization code
when you need to jump in the middle of _del + _put sequence.

--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Greg KH]

On Wed, Dec 07, 2005 at 05:18:40PM -0500, Dmitry Torokhov wrote:
> On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > On Wed, Dec 07, 2005 at 01:23:11PM -0500, Dmitry Torokhov wrote:
> > > On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > > > On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found
> > > > > that I'm missing platform_device_del that would complement
> > > > > platform_device_add. Would you object to having such a function, like
> > > > > we do for other sysfs objects? With it one can write somthing like
> > > > > this:
> > > >
> > > > Greg and myself discussed that, and we decided that it was adding
> > > > unnecessary complexity to the interface.  Maybe Greg's view has
> > > > changed?
> > > >
> > >
> > > How do you write error handling path without the _del function if
> > > platform_device_add is not the last call? you can't call
> > > platform_device_unregister() and then platform_device_put(). And I
> > > don't like to take extra references in error path or assign the
> > > pointer to NULL in teh middle of unwinding...
> >
> > The example code in the commit comments contains a complete example of
> > registering a platform device, and cleaning up should something go
> > wrong with that process.
> >
> 
> The problem with what you proposing is that one will have to code 2
> cleanup code paths - one when platform_device_add fails (in this case
> you just call platform_device_put) and another one when
> platfrom_device_add succeeds but something else fails. In the second
> case you have to use platfrom_device_unregister to release resources
> but can't use platform_device_put because the device will most likely
> be released by plaform_device_unregister. I prefer having single
> cleanup code path, like most other drivers have.
> 
> > Unregistering is just a matter of calling platform_device_unregister().
> > An unregister call is a del + put in exactly the same way as it is
> > throughout the rest of the driver model.
> >
> 
> Yes, and it works just fine everywhere except in initialization code
> when you need to jump in the middle of _del + _put sequence.

So, if you had _del, would it work easier for you?  I just objected to
it if it wasn't necessary.  I didn't want to add functions that aren't
used by anyone, but if is needed, I don't see a problem with it.

thanks,

greg k-h

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/7/05, Greg KH <greg@kroah.com> wrote:
> On Wed, Dec 07, 2005 at 05:18:40PM -0500, Dmitry Torokhov wrote:
> > On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> >
> > > Unregistering is just a matter of calling platform_device_unregister().
> > > An unregister call is a del + put in exactly the same way as it is
> > > throughout the rest of the driver model.
> > >
> >
> > Yes, and it works just fine everywhere except in initialization code
> > when you need to jump in the middle of _del + _put sequence.
>
> So, if you had _del, would it work easier for you?  I just objected to
> it if it wasn't necessary.  I didn't want to add functions that aren't
> used by anyone, but if is needed, I don't see a problem with it.
>

Yes, the I can just write:

        ...
        err = platform_driver_register(&i8042_driver);
        if (err)
                goto err_controller_cleanup;

        i8042_platform_device = platform_device_alloc("i8042", -1);
        if (!i8042_platform_device) {
                err = -ENOMEM;
                goto err_unregister_driver;
        }

        err = platform_device_add(i8042_platform_device);
        if (err)
                goto err_free_device;
        ...

        if (!have_ports) {
                err = -ENODEV;
                goto err_delete_device;
        }

        mod_timer(&i8042_timer, jiffies + I8042_POLL_PERIOD);
        return 0;

 err_delete_device:
        platform_device_del(i8042_platform_device);
 err_free_device:
        platform_device_put(i8042_platform_device);
 err_unregister_driver:
        platform_driver_unregister(&i8042_driver);
 ....

As you can see - single cleanup path..

--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Greg KH]

On Wed, Dec 07, 2005 at 05:59:24PM -0500, Dmitry Torokhov wrote:
> On 12/7/05, Greg KH <greg@kroah.com> wrote:
> > On Wed, Dec 07, 2005 at 05:18:40PM -0500, Dmitry Torokhov wrote:
> > > On 12/7/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > >
> > > > Unregistering is just a matter of calling platform_device_unregister().
> > > > An unregister call is a del + put in exactly the same way as it is
> > > > throughout the rest of the driver model.
> > > >
> > >
> > > Yes, and it works just fine everywhere except in initialization code
> > > when you need to jump in the middle of _del + _put sequence.
> >
> > So, if you had _del, would it work easier for you?  I just objected to
> > it if it wasn't necessary.  I didn't want to add functions that aren't
> > used by anyone, but if is needed, I don't see a problem with it.
> >
> 
> Yes, the I can just write:
> 
>         ...
>         err = platform_driver_register(&i8042_driver);
>         if (err)
>                 goto err_controller_cleanup;
> 
>         i8042_platform_device = platform_device_alloc("i8042", -1);
>         if (!i8042_platform_device) {
>                 err = -ENOMEM;
>                 goto err_unregister_driver;
>         }
> 
>         err = platform_device_add(i8042_platform_device);
>         if (err)
>                 goto err_free_device;
>         ...
> 
>         if (!have_ports) {
>                 err = -ENODEV;
>                 goto err_delete_device;
>         }
> 
>         mod_timer(&i8042_timer, jiffies + I8042_POLL_PERIOD);
>         return 0;
> 
>  err_delete_device:
>         platform_device_del(i8042_platform_device);
>  err_free_device:
>         platform_device_put(i8042_platform_device);
>  err_unregister_driver:
>         platform_driver_unregister(&i8042_driver);
>  ....
> 
> As you can see - single cleanup path..

Ok, that's fine with me.  Russell, any objections?

thanks,

greg k-h

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Wed, Dec 07, 2005 at 03:06:15PM -0800, Greg KH wrote:
> On Wed, Dec 07, 2005 at 05:59:24PM -0500, Dmitry Torokhov wrote:
> > Yes, the I can just write:
> > 
> >         ...
> >         err = platform_driver_register(&i8042_driver);
> >         if (err)
> >                 goto err_controller_cleanup;
> > 
> >         i8042_platform_device = platform_device_alloc("i8042", -1);
> >         if (!i8042_platform_device) {
> >                 err = -ENOMEM;
> >                 goto err_unregister_driver;
> >         }
> > 
> >         err = platform_device_add(i8042_platform_device);
> >         if (err)
> >                 goto err_free_device;
> >         ...
> > 
> >         if (!have_ports) {
> >                 err = -ENODEV;
> >                 goto err_delete_device;
> >         }
> > 
> >         mod_timer(&i8042_timer, jiffies + I8042_POLL_PERIOD);
> >         return 0;
> > 
> >  err_delete_device:
> >         platform_device_del(i8042_platform_device);
> >  err_free_device:
> >         platform_device_put(i8042_platform_device);
> >  err_unregister_driver:
> >         platform_driver_unregister(&i8042_driver);
> >  ....
> > 
> > As you can see - single cleanup path..
> 
> Ok, that's fine with me.  Russell, any objections?

None what so ever - that's mostly what I envisioned with the patch
with the _del method.  However, I didn't have an existing user for it.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Jean Delvare]

Hi Russell,

> On Wed, Dec 07, 2005 at 03:06:15PM -0800, Greg KH wrote:
> > Ok, that's fine with me.  Russell, any objections?
> 
> None what so ever - that's mostly what I envisioned with the patch
> with the _del method.  However, I didn't have an existing user for it.

Do you mean you have the code already? If it is so, could you please
provide a patch Dmitry and I can give a try to?

If not, I am willing to give it a try, if you provide some guidance. I
think I understand that platform_device_del would be the first half of
platform_device_unregister, but do we then want to rebuild
platform_device_unregister on top of platform_device_del so as to avoid
code duplication, or not?

Thanks,
-- 
Jean Delvare

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/8/05, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Russell,
>
> > On Wed, Dec 07, 2005 at 03:06:15PM -0800, Greg KH wrote:
> > > Ok, that's fine with me.  Russell, any objections?
> >
> > None what so ever - that's mostly what I envisioned with the patch
> > with the _del method.  However, I didn't have an existing user for it.
>
> Do you mean you have the code already? If it is so, could you please
> provide a patch Dmitry and I can give a try to?
>

I have the patch (my version anyway), I will send it out tonight.

> If not, I am willing to give it a try, if you provide some guidance. I
> think I understand that platform_device_del would be the first half of
> platform_device_unregister, but do we then want to rebuild
> platform_device_unregister on top of platform_device_del so as to avoid
> code duplication, or not?
>

Yes, _unregister is changed to be smply call to _del + _put.

--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Thu, Dec 08, 2005 at 09:58:15PM +0100, Jean Delvare wrote:
> Hi Russell,
> 
> > On Wed, Dec 07, 2005 at 03:06:15PM -0800, Greg KH wrote:
> > > Ok, that's fine with me.  Russell, any objections?
> > 
> > None what so ever - that's mostly what I envisioned with the patch
> > with the _del method.  However, I didn't have an existing user for it.
> 
> Do you mean you have the code already? If it is so, could you please
> provide a patch Dmitry and I can give a try to?

No; I mean I _had_ the code, and it could probably be dug out, but
subsequent patch revisions removed it.  It's probably archived in a
mail somewhere.

> If not, I am willing to give it a try, if you provide some guidance. I
> think I understand that platform_device_del would be the first half of
> platform_device_unregister, but do we then want to rebuild
> platform_device_unregister on top of platform_device_del so as to avoid
> code duplication, or not?

Yes on all counts.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Jean Delvare]

Hi Dmirty, Russell, Greg,

> On 12/7/05, Greg KH <greg@kroah.com> wrote:
> > So, if you had _del, would it work easier for you?  I just objected to
> > it if it wasn't necessary.  I didn't want to add functions that aren't
> > used by anyone, but if is needed, I don't see a problem with it.
> 
> Yes, the I can just write:
> 
>         ...
>         err = platform_driver_register(&i8042_driver);
>         if (err)
>                 goto err_controller_cleanup;
> 
>         i8042_platform_device = platform_device_alloc("i8042", -1);
>         if (!i8042_platform_device) {
>                 err = -ENOMEM;
>                 goto err_unregister_driver;
>         }
> 
>         err = platform_device_add(i8042_platform_device);
>         if (err)
>                 goto err_free_device;
>         ...
> 
>         if (!have_ports) {
>                 err = -ENODEV;
>                 goto err_delete_device;
>         }
> 
>         mod_timer(&i8042_timer, jiffies + I8042_POLL_PERIOD);
>         return 0;
> 
>  err_delete_device:
>         platform_device_del(i8042_platform_device);
>  err_free_device:
>         platform_device_put(i8042_platform_device);
>  err_unregister_driver:
>         platform_driver_unregister(&i8042_driver);
>  ....
> 
> As you can see - single cleanup path..

I second Dmitry's request here. I can't seem to possibly build a valid
error path during device registration with the current API. Having
platform_device_del() would make it possible.

BTW, doesn't this suggest that the error path in
platform_device_register_simple() is currently broken as well? If
platform_device_add() fails therein, I take it that the resources
previously allocated by platform_device_add_resources() will never be
freed.

-- 
Jean Delvare

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Thu, Dec 08, 2005 at 09:52:57PM +0100, Jean Delvare wrote:
> BTW, doesn't this suggest that the error path in
> platform_device_register_simple() is currently broken as well? If
> platform_device_add() fails therein, I take it that the resources
> previously allocated by platform_device_add_resources() will never be
> freed.

No.  If platform_device_add() fails then you platform_device_put()
it with no other action.  If it's been added, with the current
available interfaces, your only option is to
platform_device_unregister() it.

So:

- error during platform_device_alloc, no additional action necessary
- error returned by platform_device_add, you have a structure allocated
  and initialised, you platform_device_put it.
- subsequently you want to get rid of it, platform_device_unregister it,
  or alternatively platform_device_del + platform_device_put it (where
  provided.)

This is actually a generic driver model rule which can be applied to
all driver model interfaces which have the alloc/init, add, del, put,
register, unregister methods.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Jean Delvare]

Hi Russell,

> On Thu, Dec 08, 2005 at 09:52:57PM +0100, Jean Delvare wrote:
> > BTW, doesn't this suggest that the error path in
> > platform_device_register_simple() is currently broken as well? If
> > platform_device_add() fails therein, I take it that the resources
> > previously allocated by platform_device_add_resources() will never be
> > freed.
> 
> No.  If platform_device_add() fails then you platform_device_put()
> it with no other action.  If it's been added, with the current
> available interfaces, your only option is to
> platform_device_unregister() it.
> 
> So:
> 
> - error during platform_device_alloc, no additional action necessary
> - error returned by platform_device_add, you have a structure allocated
>   and initialised, you platform_device_put it.
> - subsequently you want to get rid of it, platform_device_unregister it,
>   or alternatively platform_device_del + platform_device_put it (where
>   provided.)
> 
> This is actually a generic driver model rule which can be applied to
> all driver model interfaces which have the alloc/init, add, del, put,
> register, unregister methods.

I was fine with the sequence you are describing above. The only thing
which was worrying me was platform_device_add_resources(), until I
realized that this function was really only preparing the resources for
reservation. For some reason I was erroneously thinking that it was
also requesting the resources "for real", so I was worried that
platform_device_put wouldn't release these if platform_device_add was
failing.

This all makes sense to me now. Thanks for the clarification, and sorry
for being a bit slow to figure out how the platform stuff works.

I'll post the platform driver I am currently working on later today for
comments. I'm pretty sure I'm still not using the platform
infrastructure the way it was meant to be, and would appreciate hints on
how I can do it better.

Thanks again,
-- 
Jean Delvare

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Jean Delvare]

Hi Dmirty, Russell, Greg,

Quoting myself:
> I second Dmitry's request here. I can't seem to possibly build a valid
> error path during device registration with the current API. Having
> platform_device_del() would make it possible.

I since modified the platform driver I am working on, thanks to the
guidance of Alessandro Zummo, and found that the new implementation no
longer needs platform_device_del().

My original code was registering a platform_driver, but wasn't actually
using it. In particular, the driver had no probe and remove functions.
Everything was done directly through the init and exit functions of the
module.

The new code makes proper use of platform_driver's probe and remove
function pointers. This means that the initial platform_device
registration ends with a call to platform_device_add(), and as a
consequence, the error path doesn't need platform_device_del().

So, provided that my case can be extended to others, I'd guess that
platform drivers using platform_driver.probe are likely not to need
platform_device_del(), while drivers not using platform_driver.probe
may need it. This may explain why platform_device_del wasn't needed so
far.

This raises a question. For a module which registers both the
platform_device and the matching platform_driver (as is the case for
mine), is it considered better to rely on platform_driver.probe
and .remove (as my new code does)? Or is it OK to omit these and handle
initialization and cleanup phases more direclty (as my old code did),
as this is technically possible? Using .probe and .remove looks cleaner
with regards to the driver model, but it also makes things a little
more complex.

If the goal is to always use .probe and .remove, then
platform_device_del() might become unneeded again in the long run.

Thanks,
-- 
Jean Delvare

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On Sunday 11 December 2005 14:44, Jean Delvare wrote:
> If the goal is to always use .probe and .remove, then
> platform_device_del() might become unneeded again in the long run.
> 

Only if you register just one platform device... As soon as you need to
do something else you'd need platfoprm_device_del() again.

-- 
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Dmitry Torokhov]

On 12/7/05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On 12/7/05, Dmitry Torokhov <dtor_core@ameritech.net> wrote:
> > On Monday 05 December 2005 15:27, Russell King wrote:
> > > On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> > > > The name parameter of platform_device_register_simple should be of
> > > > type const char * instead of char *, as we simply pass it to
> > > > platform_device_alloc, where it has type const char *.
> > > >
> > > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > >
> > > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > >
> > > However, I've been wondering whether we want to keep this "simple"
> > > interface around long-term given that we now have a more flexible
> > > platform device allocation interface - I don't particularly like
> > > having superfluous interfaces for folk to get confused with.
> >
> > Now that you made platform_device_alloc install default release
> > handler there is no need to have the _simple interface. I will
> > convert input devices (main users of _simple) to the new interface
> > and then we can get rid of it.
> >
>
> I have started moving drivers from the "_simple" interface and I found
> that I'm missing platform_device_del that would complement
> platform_device_add. Would you object to having such a function, like
> we do for other sysfs objects? With it one can write somthing like
> this:
>
>  err_delete_device:
>        platform_device_del(i8042_platform_device);
>  err_free_device:
>        platform_device_put(i8042_platform_device);
>  err_unregister_driver:
>        platform_driver_unregister(&i8042_driver);
>

Btw, what is the policy on placing EXPORT_SYMBOL(...). Should they all
go together (at the top or teh bottom) or after each symbol
definition? Right now platform.c mixes 2 styles...

--
Dmitry

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Randy.Dunlap]

On Wed, 7 Dec 2005, Dmitry Torokhov wrote:

> Btw, what is the policy on placing EXPORT_SYMBOL(...). Should they all
> go together (at the top or teh bottom) or after each symbol
> definition? Right now platform.c mixes 2 styles...

Not all grouped together (option 1 above), but
yes, after each symbol definition (option 2 above)...
is the current preference AFAIK.

-- 
~Randy

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Wed, Dec 07, 2005 at 10:39:36AM -0800, Randy.Dunlap wrote:
> On Wed, 7 Dec 2005, Dmitry Torokhov wrote:
> 
> > Btw, what is the policy on placing EXPORT_SYMBOL(...). Should they all
> > go together (at the top or teh bottom) or after each symbol
> > definition? Right now platform.c mixes 2 styles...
> 
> Not all grouped together (option 1 above), but
> yes, after each symbol definition (option 2 above)...
> is the current preference AFAIK.

And of course I didn't want to add extra noise by shuffling them around
needlessly, especially if we're going to be removing some of them at
some point in the future.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Jean Delvare]

Hi Russell,

> On Mon, Dec 05, 2005 at 09:23:37PM +0100, Jean Delvare wrote:
> > The name parameter of platform_device_register_simple should be of
> > type const char * instead of char *, as we simply pass it to
> > platform_device_alloc, where it has type const char *.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> 
> Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> However, I've been wondering whether we want to keep this "simple"
> interface around long-term given that we now have a more flexible
> platform device allocation interface - I don't particularly like
> having superfluous interfaces for folk to get confused with.

What is the new preferred interface? Is it already in mainline or only
in -mm? I am writing a new platform driver, and am using
platform_device_interface_simple for now. It works just fine, but if
it is going to be deprecated soon, I better update my driver before I
submit it for inclusion.

Thanks,
-- 
Jean Delvare

Re: [PATCH] Minor change to platform_device_register_simple prototype

[Russell King]

On Wed, Dec 07, 2005 at 07:50:32AM +0100, Jean Delvare wrote:
> What is the new preferred interface? Is it already in mainline or only
> in -mm? I am writing a new platform driver, and am using
> platform_device_interface_simple for now. It works just fine, but if
> it is going to be deprecated soon, I better update my driver before I
> submit it for inclusion.

It is in mainline already - see

http://www.kernel.org/git/gitweb.cgi?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=37c12e7497b6fe2b6a890814f0ff4edce696d862

where you'll find an example usage in the commit comments.

Thanks.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core