Minor change to platform_device_register_simple prototype

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 <[email protected]>

Quoted 4 lines · Jean Delvare

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 *.

Quoted 1 line

Signed-off-by Jean Delvare <[email protected]>

Acked-by Russell King <[email protected]>

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.

Quoted 5 lines · Russell King

Quoted 4 lines · Jean Delvare

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 *.

Quoted 8 lines

Quoted 1 line

Signed-off-by Jean Delvare <[email protected]>

Acked-by Russell King <[email protected]>

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.

Quoted 15 lines · Dmitry Torokhov

Quoted 5 lines · Russell King

Quoted 4 lines · Jean Delvare

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 *.

Quoted 8 lines

Quoted 1 line

Signed-off-by Jean Delvare <[email protected]>

Acked-by Russell King <[email protected]>

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.

Quoted 5 lines

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:

On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found

Quoted 4 lines

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?

Quoted 10 lines · Russell King

On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found

Quoted 4 lines

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

Quoted 12 lines · Dmitry Torokhov

Quoted 10 lines · Russell King

On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found

Quoted 4 lines

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?

Quoted 5 lines

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.

Quoted 19 lines · Russell King

Quoted 12 lines · Dmitry Torokhov

Quoted 10 lines · Russell King

On Wed, Dec 07, 2005 at 12:59:09PM -0500, Dmitry Torokhov > > I have started moving drivers from the "_simple" interface and I found

Quoted 4 lines

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?

Quoted 5 lines

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 7 lines · Dmitry Torokhov

Quoted 4 lines · Russell King

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.

Quoted 2 lines

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.

Quoted 8 lines · Greg KH

Quoted 7 lines · Dmitry Torokhov

Quoted 4 lines · Russell King

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.

Quoted 7 lines

Quoted 2 lines

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

Quoted 34 lines · Dmitry Torokhov

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);

....

Quoted 1 line

As you can see - single cleanup path..

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

Quoted 1 line

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.

Hi Russell,

Quoted 5 lines · Greg KH

Quoted 1 line

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?

Hi Russell,

Quoted 5 lines · Greg KH

Quoted 1 line

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?

Quoted 3 lines

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

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.

Quoted 5 lines

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

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

Quoted 15 lines · Dmitry Torokhov

Quoted 5 lines · Russell King

Quoted 4 lines · Jean Delvare

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 *.

Quoted 8 lines

Quoted 1 line

Signed-off-by Jean Delvare <[email protected]>

Acked-by Russell King <[email protected]>

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.

Quoted 5 lines

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

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 5 lines · Russell King

Quoted 4 lines · Jean Delvare

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 *.

Quoted 8 lines

Quoted 1 line

Signed-off-by Jean Delvare <[email protected]>

Acked-by Russell King <[email protected]>

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.

Quoted 15 lines · Dmitry Torokhov

Quoted 13 lines · Russell King

Quoted 5 lines · Jean Delvare

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 <[email protected]>

Acked-by Russell King <[email protected]>

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.

Quoted 4 lines

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 general policy on platform devices? Should they always have a corresponding driver or is it OK to leave them without one?

Quoted 7 lines

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.

Hi Dmitry,

Quoted 5 lines

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).

Quoted 2 lines

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.

Quoted 4 lines

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 *.

Quoted 1 line

Signed-off-by Jean Delvare <[email protected]>

Acked-by Russell King <[email protected]>

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.