| Message ID | 676649340.5Y2WB7IFNB@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The current platform device creation and registration code in > acpi_create_platform_device() is quite convoluted. This function > takes an ACPI device node as an argument and eventually calls > platform_device_register_resndata() to create and register a > platform device object on the basis of the information contained > in that code. However, it doesn't associate the new platform > device with the ACPI node directly, but instead it relies on > acpi_platform_notify(), called from within device_add(), to find > that ACPI node again with the help of acpi_platform_find_device() > and acpi_platform_match() and then attach the new platform device > to it. This causes an additional ACPI namespace walk to happen and > is clearly suboptimal. > > Use the observation that it is now possible to initialize the ACPI > handle of a device before calling device_add() for it to make this > code more straightforward. Namely, add a new field to struct > platform_device_info allowing us to pass the ACPI handle of interest > to platform_device_register_full(), which will then use it to > initialize the new device's ACPI handle before registering it. > This will cause acpi_platform_notify() to use the ACPI handle from > the device structure directly instead of using the .find_device() > routine provided by the device's bus type. In consequence, > acpi_platform_bus, acpi_platform_find_device(), and > acpi_platform_match() are not necessary any more, so remove them. Why can't you use the platform_data * that is already in struct device for this, instead of adding an acpi-specific field to the platform_device structure? If not that, surely there is another field in struct device that you could use that is free for this type of device? > struct platform_device_info { > struct device *parent; > + void *acpi_handle; Oh, and if I do accept this, I want a "real" structure pointer here please, not a void * "handle". That way is a slippery slope to the Windows kernel programming style :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The current platform device creation and registration code in > > acpi_create_platform_device() is quite convoluted. This function > > takes an ACPI device node as an argument and eventually calls > > platform_device_register_resndata() to create and register a > > platform device object on the basis of the information contained > > in that code. However, it doesn't associate the new platform > > device with the ACPI node directly, but instead it relies on > > acpi_platform_notify(), called from within device_add(), to find > > that ACPI node again with the help of acpi_platform_find_device() > > and acpi_platform_match() and then attach the new platform device > > to it. This causes an additional ACPI namespace walk to happen and > > is clearly suboptimal. > > > > Use the observation that it is now possible to initialize the ACPI > > handle of a device before calling device_add() for it to make this > > code more straightforward. Namely, add a new field to struct > > platform_device_info allowing us to pass the ACPI handle of interest > > to platform_device_register_full(), which will then use it to > > initialize the new device's ACPI handle before registering it. > > This will cause acpi_platform_notify() to use the ACPI handle from > > the device structure directly instead of using the .find_device() > > routine provided by the device's bus type. In consequence, > > acpi_platform_bus, acpi_platform_find_device(), and > > acpi_platform_match() are not necessary any more, so remove them. > > Why can't you use the platform_data * that is already in struct device > for this, instead of adding an acpi-specific field to the > platform_device structure? Hmm, I kind of don't understand the question. :-) Yes, we have acpi_handle in struct device (it actually is being added by a patch you've acked) and we use it. The whole point here is to streamline of the initalization of that field. > If not that, surely there is another field in struct device that you > could use that is free for this type of device? Yes, there is one and as I said above. :-) I'd be happy to use the struct device's field directly, but platform_device_register_full() allocates memory for the struct device in question, so that field actually doesn't exist yet when it is called. > > struct platform_device_info { > > struct device *parent; > > + void *acpi_handle; > > Oh, and if I do accept this, I want a "real" structure pointer here > please, not a void * "handle". That way is a slippery slope to the > Windows kernel programming style :) This is (void *), because the field being initialized is (void *). That field, in turn, is (void *), because ACPICA defines it that way. I thought about wrapping that in some more meaningless data type, but I did't find a way that would work both when CONFIG_ACPI is set and when it is not. If you have an idea how to do that in a clean way, I'd be happy to implement it. :-) Thanks, Rafael
On Monday, November 19, 2012 06:32:06 PM Rafael J. Wysocki wrote: > On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The current platform device creation and registration code in > > > acpi_create_platform_device() is quite convoluted. This function > > > takes an ACPI device node as an argument and eventually calls > > > platform_device_register_resndata() to create and register a > > > platform device object on the basis of the information contained > > > in that code. However, it doesn't associate the new platform > > > device with the ACPI node directly, but instead it relies on > > > acpi_platform_notify(), called from within device_add(), to find > > > that ACPI node again with the help of acpi_platform_find_device() > > > and acpi_platform_match() and then attach the new platform device > > > to it. This causes an additional ACPI namespace walk to happen and > > > is clearly suboptimal. > > > > > > Use the observation that it is now possible to initialize the ACPI > > > handle of a device before calling device_add() for it to make this > > > code more straightforward. Namely, add a new field to struct > > > platform_device_info allowing us to pass the ACPI handle of interest > > > to platform_device_register_full(), which will then use it to > > > initialize the new device's ACPI handle before registering it. > > > This will cause acpi_platform_notify() to use the ACPI handle from > > > the device structure directly instead of using the .find_device() > > > routine provided by the device's bus type. In consequence, > > > acpi_platform_bus, acpi_platform_find_device(), and > > > acpi_platform_match() are not necessary any more, so remove them. > > > > Why can't you use the platform_data * that is already in struct device > > for this, instead of adding an acpi-specific field to the > > platform_device structure? > > Hmm, I kind of don't understand the question. :-) > > Yes, we have acpi_handle in struct device (it actually is being added by a > patch you've acked) and we use it. The whole point here is to streamline > of the initalization of that field. > > > If not that, surely there is another field in struct device that you > > could use that is free for this type of device? > > Yes, there is one and as I said above. :-) > > I'd be happy to use the struct device's field directly, but > platform_device_register_full() allocates memory for the struct device in > question, so that field actually doesn't exist yet when it is called. > > > > struct platform_device_info { > > > struct device *parent; > > > + void *acpi_handle; > > > > Oh, and if I do accept this, I want a "real" structure pointer here > > please, not a void * "handle". That way is a slippery slope to the > > Windows kernel programming style :) > > This is (void *), because the field being initialized is (void *). That field, > in turn, is (void *), because ACPICA defines it that way. I thought about > wrapping that in some more meaningless data type, but I did't find a way s/meaningless/meaningful/ (/me hides)
On Monday, November 19, 2012 06:32:06 PM Rafael J. Wysocki wrote: > On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The current platform device creation and registration code in > > > acpi_create_platform_device() is quite convoluted. This function > > > takes an ACPI device node as an argument and eventually calls > > > platform_device_register_resndata() to create and register a > > > platform device object on the basis of the information contained > > > in that code. However, it doesn't associate the new platform > > > device with the ACPI node directly, but instead it relies on > > > acpi_platform_notify(), called from within device_add(), to find > > > that ACPI node again with the help of acpi_platform_find_device() > > > and acpi_platform_match() and then attach the new platform device > > > to it. This causes an additional ACPI namespace walk to happen and > > > is clearly suboptimal. > > > > > > Use the observation that it is now possible to initialize the ACPI > > > handle of a device before calling device_add() for it to make this > > > code more straightforward. Namely, add a new field to struct > > > platform_device_info allowing us to pass the ACPI handle of interest > > > to platform_device_register_full(), which will then use it to > > > initialize the new device's ACPI handle before registering it. > > > This will cause acpi_platform_notify() to use the ACPI handle from > > > the device structure directly instead of using the .find_device() > > > routine provided by the device's bus type. In consequence, > > > acpi_platform_bus, acpi_platform_find_device(), and > > > acpi_platform_match() are not necessary any more, so remove them. > > > > Why can't you use the platform_data * that is already in struct device > > for this, instead of adding an acpi-specific field to the > > platform_device structure? > > Hmm, I kind of don't understand the question. :-) > > Yes, we have acpi_handle in struct device (it actually is being added by a > patch you've acked) and we use it. The whole point here is to streamline > of the initalization of that field. > > > If not that, surely there is another field in struct device that you > > could use that is free for this type of device? > > Yes, there is one and as I said above. :-) > > I'd be happy to use the struct device's field directly, but > platform_device_register_full() allocates memory for the struct device in > question, so that field actually doesn't exist yet when it is called. So what happens is we want to use platform_device_register_full(), because it initializes a struct platform_device for use and registers it for us, so we don't need to worry about all that. However, platform_device_register_full() also creates the struct platform_device it registers and we need to let it know what value to put into the acpi_handle field of that struct platform_device object's struct device component. That's what this all is about. :-) Thanks, Rafael
On Monday, November 19, 2012 06:45:22 PM Rafael J. Wysocki wrote: > On Monday, November 19, 2012 06:32:06 PM Rafael J. Wysocki wrote: > > On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > > > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > The current platform device creation and registration code in > > > > acpi_create_platform_device() is quite convoluted. This function > > > > takes an ACPI device node as an argument and eventually calls > > > > platform_device_register_resndata() to create and register a > > > > platform device object on the basis of the information contained > > > > in that code. However, it doesn't associate the new platform > > > > device with the ACPI node directly, but instead it relies on > > > > acpi_platform_notify(), called from within device_add(), to find > > > > that ACPI node again with the help of acpi_platform_find_device() > > > > and acpi_platform_match() and then attach the new platform device > > > > to it. This causes an additional ACPI namespace walk to happen and > > > > is clearly suboptimal. > > > > > > > > Use the observation that it is now possible to initialize the ACPI > > > > handle of a device before calling device_add() for it to make this > > > > code more straightforward. Namely, add a new field to struct > > > > platform_device_info allowing us to pass the ACPI handle of interest > > > > to platform_device_register_full(), which will then use it to > > > > initialize the new device's ACPI handle before registering it. > > > > This will cause acpi_platform_notify() to use the ACPI handle from > > > > the device structure directly instead of using the .find_device() > > > > routine provided by the device's bus type. In consequence, > > > > acpi_platform_bus, acpi_platform_find_device(), and > > > > acpi_platform_match() are not necessary any more, so remove them. > > > > > > Why can't you use the platform_data * that is already in struct device > > > for this, instead of adding an acpi-specific field to the > > > platform_device structure? > > > > Hmm, I kind of don't understand the question. :-) > > > > Yes, we have acpi_handle in struct device (it actually is being added by a > > patch you've acked) and we use it. The whole point here is to streamline > > of the initalization of that field. > > > > > If not that, surely there is another field in struct device that you > > > could use that is free for this type of device? > > > > Yes, there is one and as I said above. :-) > > > > I'd be happy to use the struct device's field directly, but > > platform_device_register_full() allocates memory for the struct device in > > question, so that field actually doesn't exist yet when it is called. > > > > > > struct platform_device_info { > > > > struct device *parent; > > > > + void *acpi_handle; > > > > > > Oh, and if I do accept this, I want a "real" structure pointer here > > > please, not a void * "handle". That way is a slippery slope to the > > > Windows kernel programming style :) > > > > This is (void *), because the field being initialized is (void *). That field, > > in turn, is (void *), because ACPICA defines it that way. I thought about > > wrapping that in some more meaningless data type, but I did't find a way > > s/meaningless/meaningful/ Well, perhaps I'll describe the problem to you, maybe you can help. :-) So, we want to have acpi_handle (or acpi_node) in addition to of_node in struct device (to be used in the analogous way plus for the execution of AML methods), but we don't want all users of device.h to have to include ACPI headers where the acpi_handle data type is defined. For this reason, we're using (void *) as its data type now, which let's say I'm not really happy with. I've been thinking about that for quite a while, though, and I'm not really sure what to do about that. Perhaps we could define something like struct acpi_dev_node { #ifdef CONFIG_ACPI void *handle; #endif }; in device.h and use that as "struct acpi_dev_node acpi_node;" in struct device. Then, we could add the following macro #ifdef CONFIG_ACPI #define ACPI_HANDLE(dev) ((dev)->acpi_node.handle) #else #define ACPI_HANDLE(dev) (NULL) #endif and redefine DEVICE_ACPI_HANDLE(dev) as ((acpi_handle)ACPI_HANDLE(dev)). Then, the $subject patch would add "struct acpi_dev_node acpi_node;" to struct platform_device_info and use ACPI_HANDLE(dev) instead of accessing the struct device's field directly. I wonder what you think? Rafael
On Mon, Nov 19, 2012 at 09:44:21PM +0100, Rafael J. Wysocki wrote: > So, we want to have acpi_handle (or acpi_node) in addition to of_node in struct > device (to be used in the analogous way plus for the execution of AML methods), > but we don't want all users of device.h to have to include ACPI headers > where the acpi_handle data type is defined. For this reason, we're using > (void *) as its data type now, which let's say I'm not really happy with. > > I've been thinking about that for quite a while, though, and I'm not really > sure what to do about that. Perhaps we could define something like > > struct acpi_dev_node { > #ifdef CONFIG_ACPI > void *handle; > #endif > }; > > in device.h and use that as "struct acpi_dev_node acpi_node;" in struct device. > Then, we could add the following macro > > #ifdef CONFIG_ACPI > #define ACPI_HANDLE(dev) ((dev)->acpi_node.handle) > #else > #define ACPI_HANDLE(dev) (NULL) > #endif > > and redefine DEVICE_ACPI_HANDLE(dev) as ((acpi_handle)ACPI_HANDLE(dev)). > > Then, the $subject patch would add "struct acpi_dev_node acpi_node;" to > struct platform_device_info and use ACPI_HANDLE(dev) instead of accessing > the struct device's field directly. In addition to struct platform_device_info, we are also going to add similar to struct i2c_board_info. There already is of_node pointer so I was thinking to add acpi_handle like you did for platform_device. Type of that pointer of course needs to be figured out :) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 19, 2012 11:05:28 PM Mika Westerberg wrote: > On Mon, Nov 19, 2012 at 09:44:21PM +0100, Rafael J. Wysocki wrote: > > So, we want to have acpi_handle (or acpi_node) in addition to of_node in struct > > device (to be used in the analogous way plus for the execution of AML methods), > > but we don't want all users of device.h to have to include ACPI headers > > where the acpi_handle data type is defined. For this reason, we're using > > (void *) as its data type now, which let's say I'm not really happy with. > > > > I've been thinking about that for quite a while, though, and I'm not really > > sure what to do about that. Perhaps we could define something like > > > > struct acpi_dev_node { > > #ifdef CONFIG_ACPI > > void *handle; > > #endif > > }; > > > > in device.h and use that as "struct acpi_dev_node acpi_node;" in struct device. > > Then, we could add the following macro > > > > #ifdef CONFIG_ACPI > > #define ACPI_HANDLE(dev) ((dev)->acpi_node.handle) > > #else > > #define ACPI_HANDLE(dev) (NULL) > > #endif > > > > and redefine DEVICE_ACPI_HANDLE(dev) as ((acpi_handle)ACPI_HANDLE(dev)). > > > > Then, the $subject patch would add "struct acpi_dev_node acpi_node;" to > > struct platform_device_info and use ACPI_HANDLE(dev) instead of accessing > > the struct device's field directly. > > In addition to struct platform_device_info, we are also going to add > similar to struct i2c_board_info. There already is of_node pointer so I was > thinking to add acpi_handle like you did for platform_device. Yeah, that's kind of something that comes to mind immediately. :-) > Type of that pointer of course needs to be figured out :) Yup.
On Mon, Nov 19, 2012 at 06:32:06PM +0100, Rafael J. Wysocki wrote: > On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The current platform device creation and registration code in > > > acpi_create_platform_device() is quite convoluted. This function > > > takes an ACPI device node as an argument and eventually calls > > > platform_device_register_resndata() to create and register a > > > platform device object on the basis of the information contained > > > in that code. However, it doesn't associate the new platform > > > device with the ACPI node directly, but instead it relies on > > > acpi_platform_notify(), called from within device_add(), to find > > > that ACPI node again with the help of acpi_platform_find_device() > > > and acpi_platform_match() and then attach the new platform device > > > to it. This causes an additional ACPI namespace walk to happen and > > > is clearly suboptimal. > > > > > > Use the observation that it is now possible to initialize the ACPI > > > handle of a device before calling device_add() for it to make this > > > code more straightforward. Namely, add a new field to struct > > > platform_device_info allowing us to pass the ACPI handle of interest > > > to platform_device_register_full(), which will then use it to > > > initialize the new device's ACPI handle before registering it. > > > This will cause acpi_platform_notify() to use the ACPI handle from > > > the device structure directly instead of using the .find_device() > > > routine provided by the device's bus type. In consequence, > > > acpi_platform_bus, acpi_platform_find_device(), and > > > acpi_platform_match() are not necessary any more, so remove them. > > > > Why can't you use the platform_data * that is already in struct device > > for this, instead of adding an acpi-specific field to the > > platform_device structure? > > Hmm, I kind of don't understand the question. :-) > > Yes, we have acpi_handle in struct device (it actually is being added by a > patch you've acked) and we use it. The whole point here is to streamline > of the initalization of that field. Ok, but then why would you need it again in platform device? That's what is confusing me. > > If not that, surely there is another field in struct device that you > > could use that is free for this type of device? > > Yes, there is one and as I said above. :-) > > I'd be happy to use the struct device's field directly, but > platform_device_register_full() allocates memory for the struct device in > question, so that field actually doesn't exist yet when it is called. Ah, this is in the _info structure, not the platform_device structure. Doh, sorry about that, I totally missed that. Nevermind about my objections. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 19, 2012 at 09:44:21PM +0100, Rafael J. Wysocki wrote: > On Monday, November 19, 2012 06:45:22 PM Rafael J. Wysocki wrote: > > On Monday, November 19, 2012 06:32:06 PM Rafael J. Wysocki wrote: > > > On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > > > > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > The current platform device creation and registration code in > > > > > acpi_create_platform_device() is quite convoluted. This function > > > > > takes an ACPI device node as an argument and eventually calls > > > > > platform_device_register_resndata() to create and register a > > > > > platform device object on the basis of the information contained > > > > > in that code. However, it doesn't associate the new platform > > > > > device with the ACPI node directly, but instead it relies on > > > > > acpi_platform_notify(), called from within device_add(), to find > > > > > that ACPI node again with the help of acpi_platform_find_device() > > > > > and acpi_platform_match() and then attach the new platform device > > > > > to it. This causes an additional ACPI namespace walk to happen and > > > > > is clearly suboptimal. > > > > > > > > > > Use the observation that it is now possible to initialize the ACPI > > > > > handle of a device before calling device_add() for it to make this > > > > > code more straightforward. Namely, add a new field to struct > > > > > platform_device_info allowing us to pass the ACPI handle of interest > > > > > to platform_device_register_full(), which will then use it to > > > > > initialize the new device's ACPI handle before registering it. > > > > > This will cause acpi_platform_notify() to use the ACPI handle from > > > > > the device structure directly instead of using the .find_device() > > > > > routine provided by the device's bus type. In consequence, > > > > > acpi_platform_bus, acpi_platform_find_device(), and > > > > > acpi_platform_match() are not necessary any more, so remove them. > > > > > > > > Why can't you use the platform_data * that is already in struct device > > > > for this, instead of adding an acpi-specific field to the > > > > platform_device structure? > > > > > > Hmm, I kind of don't understand the question. :-) > > > > > > Yes, we have acpi_handle in struct device (it actually is being added by a > > > patch you've acked) and we use it. The whole point here is to streamline > > > of the initalization of that field. > > > > > > > If not that, surely there is another field in struct device that you > > > > could use that is free for this type of device? > > > > > > Yes, there is one and as I said above. :-) > > > > > > I'd be happy to use the struct device's field directly, but > > > platform_device_register_full() allocates memory for the struct device in > > > question, so that field actually doesn't exist yet when it is called. > > > > > > > > struct platform_device_info { > > > > > struct device *parent; > > > > > + void *acpi_handle; > > > > > > > > Oh, and if I do accept this, I want a "real" structure pointer here > > > > please, not a void * "handle". That way is a slippery slope to the > > > > Windows kernel programming style :) > > > > > > This is (void *), because the field being initialized is (void *). That field, > > > in turn, is (void *), because ACPICA defines it that way. I thought about > > > wrapping that in some more meaningless data type, but I did't find a way > > > > s/meaningless/meaningful/ > > Well, perhaps I'll describe the problem to you, maybe you can help. :-) > > So, we want to have acpi_handle (or acpi_node) in addition to of_node in struct > device (to be used in the analogous way plus for the execution of AML methods), > but we don't want all users of device.h to have to include ACPI headers > where the acpi_handle data type is defined. For this reason, we're using > (void *) as its data type now, which let's say I'm not really happy with. > > I've been thinking about that for quite a while, though, and I'm not really > sure what to do about that. Perhaps we could define something like > > struct acpi_dev_node { > #ifdef CONFIG_ACPI > void *handle; > #endif > }; > > in device.h and use that as "struct acpi_dev_node acpi_node;" in struct device. > Then, we could add the following macro > > #ifdef CONFIG_ACPI > #define ACPI_HANDLE(dev) ((dev)->acpi_node.handle) > #else > #define ACPI_HANDLE(dev) (NULL) > #endif > > and redefine DEVICE_ACPI_HANDLE(dev) as ((acpi_handle)ACPI_HANDLE(dev)). > > Then, the $subject patch would add "struct acpi_dev_node acpi_node;" to > struct platform_device_info and use ACPI_HANDLE(dev) instead of accessing > the struct device's field directly. > > I wonder what you think? I like the hack of using an empty structure here, that's fine with me, and makes me feel a little bit better about the whole "void *" stuff. If you respin the patch with this, I'll ack it. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 19, 2012 02:32:21 PM Greg Kroah-Hartman wrote: > On Mon, Nov 19, 2012 at 09:44:21PM +0100, Rafael J. Wysocki wrote: > > On Monday, November 19, 2012 06:45:22 PM Rafael J. Wysocki wrote: > > > On Monday, November 19, 2012 06:32:06 PM Rafael J. Wysocki wrote: > > > > On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > > > > > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > > > The current platform device creation and registration code in > > > > > > acpi_create_platform_device() is quite convoluted. This function > > > > > > takes an ACPI device node as an argument and eventually calls > > > > > > platform_device_register_resndata() to create and register a > > > > > > platform device object on the basis of the information contained > > > > > > in that code. However, it doesn't associate the new platform > > > > > > device with the ACPI node directly, but instead it relies on > > > > > > acpi_platform_notify(), called from within device_add(), to find > > > > > > that ACPI node again with the help of acpi_platform_find_device() > > > > > > and acpi_platform_match() and then attach the new platform device > > > > > > to it. This causes an additional ACPI namespace walk to happen and > > > > > > is clearly suboptimal. > > > > > > > > > > > > Use the observation that it is now possible to initialize the ACPI > > > > > > handle of a device before calling device_add() for it to make this > > > > > > code more straightforward. Namely, add a new field to struct > > > > > > platform_device_info allowing us to pass the ACPI handle of interest > > > > > > to platform_device_register_full(), which will then use it to > > > > > > initialize the new device's ACPI handle before registering it. > > > > > > This will cause acpi_platform_notify() to use the ACPI handle from > > > > > > the device structure directly instead of using the .find_device() > > > > > > routine provided by the device's bus type. In consequence, > > > > > > acpi_platform_bus, acpi_platform_find_device(), and > > > > > > acpi_platform_match() are not necessary any more, so remove them. > > > > > > > > > > Why can't you use the platform_data * that is already in struct device > > > > > for this, instead of adding an acpi-specific field to the > > > > > platform_device structure? > > > > > > > > Hmm, I kind of don't understand the question. :-) > > > > > > > > Yes, we have acpi_handle in struct device (it actually is being added by a > > > > patch you've acked) and we use it. The whole point here is to streamline > > > > of the initalization of that field. > > > > > > > > > If not that, surely there is another field in struct device that you > > > > > could use that is free for this type of device? > > > > > > > > Yes, there is one and as I said above. :-) > > > > > > > > I'd be happy to use the struct device's field directly, but > > > > platform_device_register_full() allocates memory for the struct device in > > > > question, so that field actually doesn't exist yet when it is called. > > > > > > > > > > struct platform_device_info { > > > > > > struct device *parent; > > > > > > + void *acpi_handle; > > > > > > > > > > Oh, and if I do accept this, I want a "real" structure pointer here > > > > > please, not a void * "handle". That way is a slippery slope to the > > > > > Windows kernel programming style :) > > > > > > > > This is (void *), because the field being initialized is (void *). That field, > > > > in turn, is (void *), because ACPICA defines it that way. I thought about > > > > wrapping that in some more meaningless data type, but I did't find a way > > > > > > s/meaningless/meaningful/ > > > > Well, perhaps I'll describe the problem to you, maybe you can help. :-) > > > > So, we want to have acpi_handle (or acpi_node) in addition to of_node in struct > > device (to be used in the analogous way plus for the execution of AML methods), > > but we don't want all users of device.h to have to include ACPI headers > > where the acpi_handle data type is defined. For this reason, we're using > > (void *) as its data type now, which let's say I'm not really happy with. > > > > I've been thinking about that for quite a while, though, and I'm not really > > sure what to do about that. Perhaps we could define something like > > > > struct acpi_dev_node { > > #ifdef CONFIG_ACPI > > void *handle; > > #endif > > }; > > > > in device.h and use that as "struct acpi_dev_node acpi_node;" in struct device. > > Then, we could add the following macro > > > > #ifdef CONFIG_ACPI > > #define ACPI_HANDLE(dev) ((dev)->acpi_node.handle) > > #else > > #define ACPI_HANDLE(dev) (NULL) > > #endif > > > > and redefine DEVICE_ACPI_HANDLE(dev) as ((acpi_handle)ACPI_HANDLE(dev)). > > > > Then, the $subject patch would add "struct acpi_dev_node acpi_node;" to > > struct platform_device_info and use ACPI_HANDLE(dev) instead of accessing > > the struct device's field directly. > > > > I wonder what you think? > > I like the hack of using an empty structure here, that's fine with me, > and makes me feel a little bit better about the whole "void *" stuff. > If you respin the patch with this, I'll ack it. I will, but I think I'll cut two patches instead, one introducing that stuff above and the other as a replacement for the $subject one on top of that. Thanks, Rafael
On Monday, November 19, 2012 02:31:35 PM Greg Kroah-Hartman wrote: > On Mon, Nov 19, 2012 at 06:32:06PM +0100, Rafael J. Wysocki wrote: > > On Monday, November 19, 2012 08:23:34 AM Greg Kroah-Hartman wrote: > > > On Sun, Nov 18, 2012 at 10:13:59PM +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > The current platform device creation and registration code in > > > > acpi_create_platform_device() is quite convoluted. This function > > > > takes an ACPI device node as an argument and eventually calls > > > > platform_device_register_resndata() to create and register a > > > > platform device object on the basis of the information contained > > > > in that code. However, it doesn't associate the new platform > > > > device with the ACPI node directly, but instead it relies on > > > > acpi_platform_notify(), called from within device_add(), to find > > > > that ACPI node again with the help of acpi_platform_find_device() > > > > and acpi_platform_match() and then attach the new platform device > > > > to it. This causes an additional ACPI namespace walk to happen and > > > > is clearly suboptimal. > > > > > > > > Use the observation that it is now possible to initialize the ACPI > > > > handle of a device before calling device_add() for it to make this > > > > code more straightforward. Namely, add a new field to struct > > > > platform_device_info allowing us to pass the ACPI handle of interest > > > > to platform_device_register_full(), which will then use it to > > > > initialize the new device's ACPI handle before registering it. > > > > This will cause acpi_platform_notify() to use the ACPI handle from > > > > the device structure directly instead of using the .find_device() > > > > routine provided by the device's bus type. In consequence, > > > > acpi_platform_bus, acpi_platform_find_device(), and > > > > acpi_platform_match() are not necessary any more, so remove them. > > > > > > Why can't you use the platform_data * that is already in struct device > > > for this, instead of adding an acpi-specific field to the > > > platform_device structure? > > > > Hmm, I kind of don't understand the question. :-) > > > > Yes, we have acpi_handle in struct device (it actually is being added by a > > patch you've acked) and we use it. The whole point here is to streamline > > of the initalization of that field. > > Ok, but then why would you need it again in platform device? That's > what is confusing me. > > > > If not that, surely there is another field in struct device that you > > > could use that is free for this type of device? > > > > Yes, there is one and as I said above. :-) > > > > I'd be happy to use the struct device's field directly, but > > platform_device_register_full() allocates memory for the struct device in > > question, so that field actually doesn't exist yet when it is called. > > Ah, this is in the _info structure, not the platform_device structure. > Doh, sorry about that, I totally missed that. Nevermind about my > objections. Cool, thanks! :-) I'll try to fight with the (void *) things a bit, though. Thanks, Rafael
Index: linux/include/linux/platform_device.h =================================================================== --- linux.orig/include/linux/platform_device.h +++ linux/include/linux/platform_device.h @@ -55,6 +55,7 @@ extern int platform_add_devices(struct p struct platform_device_info { struct device *parent; + void *acpi_handle; const char *name; int id; Index: linux/drivers/acpi/acpi_platform.c =================================================================== --- linux.orig/drivers/acpi/acpi_platform.c +++ linux/drivers/acpi/acpi_platform.c @@ -33,7 +33,7 @@ struct platform_device *acpi_create_plat { struct platform_device *pdev = NULL; struct acpi_device *acpi_parent; - struct device *parent = NULL; + struct platform_device_info pdevinfo; struct resource_list_entry *rentry; struct list_head resource_list; struct resource *resources; @@ -60,11 +60,13 @@ struct platform_device *acpi_create_plat acpi_dev_free_resource_list(&resource_list); + memset(&pdevinfo, 0, sizeof(pdevinfo)); /* * If the ACPI node has a parent and that parent has a physical device * attached to it, that physical device should be the parent of the * platform device we are about to create. */ + pdevinfo.parent = NULL; acpi_parent = adev->parent; if (acpi_parent) { struct acpi_device_physical_node *entry; @@ -76,12 +78,16 @@ struct platform_device *acpi_create_plat entry = list_first_entry(list, struct acpi_device_physical_node, node); - parent = entry->dev; + pdevinfo.parent = entry->dev; } mutex_unlock(&acpi_parent->physical_node_lock); } - pdev = platform_device_register_resndata(parent, dev_name(&adev->dev), - -1, resources, count, NULL, 0); + pdevinfo.name = dev_name(&adev->dev); + pdevinfo.id = -1; + pdevinfo.res = resources; + pdevinfo.num_res = count; + pdevinfo.acpi_handle = adev->handle; + pdev = platform_device_register_full(&pdevinfo); if (IS_ERR(pdev)) { dev_err(&adev->dev, "platform device creation failed: %ld\n", PTR_ERR(pdev)); @@ -94,65 +100,3 @@ struct platform_device *acpi_create_plat kfree(resources); return pdev; } - -static acpi_status acpi_platform_match(acpi_handle handle, u32 depth, - void *data, void **return_value) -{ - struct platform_device *pdev = data; - struct acpi_device *adev; - acpi_status status; - - status = acpi_bus_get_device(handle, &adev); - if (ACPI_FAILURE(status)) - return status; - - /* Skip ACPI devices that have physical device attached */ - if (adev->physical_node_count) - return AE_OK; - - if (!strcmp(dev_name(&pdev->dev), dev_name(&adev->dev))) { - *(acpi_handle *)return_value = handle; - return AE_CTRL_TERMINATE; - } - - return AE_OK; -} - -static int acpi_platform_find_device(struct device *dev, acpi_handle *handle) -{ - struct platform_device *pdev = to_platform_device(dev); - char *name, *tmp, *hid; - - /* - * The platform device is named using the ACPI device name - * _HID:INSTANCE so we strip the INSTANCE out in order to find the - * correct device using its _HID. - */ - name = kstrdup(dev_name(dev), GFP_KERNEL); - if (!name) - return -ENOMEM; - - tmp = name; - hid = strsep(&tmp, ":"); - if (!hid) { - kfree(name); - return -ENODEV; - } - - *handle = NULL; - acpi_get_devices(hid, acpi_platform_match, pdev, handle); - - kfree(name); - return *handle ? 0 : -ENODEV; -} - -static struct acpi_bus_type acpi_platform_bus = { - .bus = &platform_bus_type, - .find_device = acpi_platform_find_device, -}; - -static int __init acpi_platform_init(void) -{ - return register_acpi_bus_type(&acpi_platform_bus); -} -arch_initcall(acpi_platform_init); Index: linux/drivers/base/platform.c =================================================================== --- linux.orig/drivers/base/platform.c +++ linux/drivers/base/platform.c @@ -437,6 +437,7 @@ struct platform_device *platform_device_ goto err_alloc; pdev->dev.parent = pdevinfo->parent; + pdev->dev.acpi_handle = pdevinfo->acpi_handle; if (pdevinfo->dma_mask) { /* @@ -467,6 +468,7 @@ struct platform_device *platform_device_ ret = platform_device_add(pdev); if (ret) { err: + pdev->dev.acpi_handle = NULL; kfree(pdev->dev.dma_mask); err_alloc: