Suspend support for PNP bus.]
RFC PATCH 1/1 Suspend support for PNP bus.]
Perhaps Mr Morton can have a look? This patch is probably -mm material anyhow.
Rgds
Pierre
Add support for suspending devices connected to the PNP bus. New callbacks are added for the drivers and the PNP hardware layer is told to disable the device during the suspend.
---
drivers/pnp/driver.c | 51 +++++++++++++++++++++++++++++++- drivers/pnp/manager.c | 78 +++++++++++++++++++++++++++++++++++++------------ include/linux/pnp.h | 10 +++++- 3 files changed, 116 insertions(+), 23 deletions(-)
drivers/pnp/driver.c +49 -2
@@ -146,10 +146,57 @@ static int pnp_bus_match(struct device *return 1 ; } +static int pnp_bus_suspend (struct device *dev,pm_message_t state) +{ +struct pnp_dev * pnp_dev =to_pnp_dev (dev); +struct pnp_driver * drv = pnp_dev->driver; +int error; + +if (!drv) +return 0 ; + +if (drv->suspend) { + error = drv->suspend (pnp_dev, state); +if (error) +return error; + } + +if (!(drv->flags &PNP_DRIVER_RES_DO_NOT_CHANGE ) && +pnp_can_disable (pnp_dev)) { + error =pnp_stop_dev (pnp_dev); +if (error) +return error; + } + +return 0 ; +} + +static int pnp_bus_resume (struct device *dev) +{ +struct pnp_dev * pnp_dev =to_pnp_dev (dev); +struct pnp_driver * drv = pnp_dev->driver; +int error; + +if (!drv) +return 0 ; + +if (!(drv->flags &PNP_DRIVER_RES_DO_NOT_CHANGE )) { + error =pnp_start_dev (pnp_dev); +if (error) +return error; + } + +if (drv->resume) +return drv->resume (pnp_dev); + +return 0 ; +}struct bus_type pnp_bus_type = { - .name ="pnp" , - .match = pnp_bus_match, + .name ="pnp" , + .match = pnp_bus_match, + .suspend = pnp_bus_suspend, + .resume = pnp_bus_resume, };
drivers/pnp/manager.c +59 -19
118 lines not on this page · raw message
include/linux/pnp.h +8 -2
@@ -292,8 +292,10 @@ struct pnp_driver {char * name;const struct pnp_device_id *id_table;unsigned int flags; -int (*probe) (struct pnp_dev *dev,const struct pnp_device_id *dev_id); -void (*remove) (struct pnp_dev *dev); +int (*probe) (struct pnp_dev *dev,const struct pnp_device_id *dev_id); +void (*remove) (struct pnp_dev *dev); +int (*suspend) (struct pnp_dev *dev,pm_message_t state); +int (*resume) (struct pnp_dev *dev);struct device_driver driver; };
@@ -381,6 +383,8 @@ void pnp_init_resource_table(struct pnp_int pnp_manual_config_dev (struct pnp_dev *dev,struct pnp_resource_table *res,int mode);int pnp_auto_config_dev (struct pnp_dev *dev);int pnp_validate_config (struct pnp_dev *dev); +int pnp_start_dev (struct pnp_dev *dev); +int pnp_stop_dev (struct pnp_dev *dev);int pnp_activate_dev (struct pnp_dev *dev);int pnp_disable_dev (struct pnp_dev *dev);void pnp_resource_change (struct resource *resource,unsigned long start,unsigned long size);
@@ -425,6 +429,8 @@ static inline void pnp_init_resource_tabstatic inline int pnp_manual_config_dev (struct pnp_dev *dev,struct pnp_resource_table *res,int mode) {return -ENODEV ; }static inline int pnp_auto_config_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline int pnp_validate_config (struct pnp_dev *dev) {return -ENODEV ; } +static inline int pnp_start_dev (struct pnp_dev *dev) {return -ENODEV ; } +static inline int pnp_stop_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline int pnp_activate_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline int pnp_disable_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline void pnp_resource_change (struct resource *resource,unsigned long start,unsigned long size) { }
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
drivers/pnp/manager.c int pnp_auto_config_dev(struct pnp_dev * · line 480
Quoted 5 lines · Pierre Ossman
drivers/pnp/manager.c +5
-0+ +int pnp_start_dev (struct pnp_dev *dev) +{ +if (!pnp_can_write (dev)) { +pnp_info ("Device %s does not supported activation." , dev->dev.bus_id);
"...does not support...", there is no "ed" at the end.
Review
Quoted 2 lines
(quoted diff) +2
-0+ return -EINVAL; + }
Hmm, would'nt presence of such device stop suspend process? It will cause pnp_bus_resume to fail too. Perhaps returning 0 in this case is better.
drivers/pnp/manager.c int pnp_auto_config_dev(struct pnp_dev * · line 505
Quoted 6 lines
drivers/pnp/manager.c +6
-0+ +int pnp_stop_dev (struct pnp_dev *dev) +{ +if (!pnp_can_disable (dev)) { +pnp_info ("Device %s does not supported disabling." , dev->dev.bus_id); +return -EINVAL ;
Same here. No "ed" and -EINVAL will hurt.
--
Dmitry
Quoted 12 lines · Dmitry Torokhov
drivers/pnp/manager.c int pnp_auto_config_dev(struct pnp_dev * · line 480
Quoted 6 lines · Pierre Ossman
drivers/pnp/manager.c +5
-0+ +int pnp_start_dev (struct pnp_dev *dev) +{ +if (!pnp_can_write (dev)) { +pnp_info ("Device %s does not supported activation." , dev->dev.bus_id);"...does not support...", there is no "ed" at the end.
That's just code that's been moved around. But I suppose a speling fix could be included in the same patch. :)
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ return -EINVAL; + }Hmm, would'nt presence of such device stop suspend process? It will cause
pnp_bus_resumeto fail too. Perhaps returning 0 in this case is better.
The problem is that this code is also visited from pnp_activate_dev() & co where this return value is needed. For pnp_stop_dev() the same check (pnp_can_disable()) is performed in the suspend routine to avoid that particular problem. For resume my assumption was that a device that doesn't support activation will not have a driver attached to it. Perhaps this is wrong?
Rgds
Pierre
Quoted 8 lines · Pierre Ossman
Quoted 6 lines · Dmitry Torokhov
Hmm, would'nt presence of such device stop suspend process? It will cause
pnp_bus_resumeto fail too. Perhaps returning 0 in this case is better.
Quoted 7 lines
The problem is that this code is also visited from
pnp_activate_dev()& co where this return value is needed. Forpnp_stop_dev()the same check (pnp_can_disable()) is performed in the suspend routine to avoid that particular problem. For resume my assumption was that a device that doesn't support activation will not have a driver attached to it. Perhaps this is wrong?
i8042 registers drivers for keyboard and AUX ports to gather information whether the ports are present and what IRQ and IO ports shoudl be used to access them. And I have seen a few boxes that do not alloe [de]activate these devices.
--
Dmitry
Quoted 19 lines · Dmitry Torokhov
Quoted 8 lines · Pierre Ossman
Quoted 6 lines · Dmitry Torokhov
Hmm, would'nt presence of such device stop suspend process? It will cause
pnp_bus_resumeto fail too. Perhaps returning 0 in this case is better.Quoted 8 lines
The problem is that this code is also visited from
pnp_activate_dev()& co where this return value is needed. Forpnp_stop_dev()the same check (pnp_can_disable()) is performed in the suspend routine to avoid that particular problem. For resume my assumption was that a device that doesn't support activation will not have a driver attached to it. Perhaps this is wrong?
Quoted 6 lines
i8042 registers drivers for keyboard and AUX ports to gather information whether the ports are present and what IRQ and IO ports shoudl be used to access them. And I have seen a few boxes that do not alloe [de]activate these devices.
But the activation is performed by the PNP layer when the driver is matched with a driver. So the driver isn't really responsible for the activation. It can prevent activation through PNP_DRIVER_RES_DO_NOT_CHANGE though, but that flag is also checked in the suspend routines.
Rgds
Pierre
Quoted 27 lines · Pierre Ossman
Quoted 19 lines · Dmitry Torokhov
Quoted 8 lines · Pierre Ossman
Quoted 6 lines · Dmitry Torokhov
Hmm, would'nt presence of such device stop suspend process? It will cause
pnp_bus_resumeto fail too. Perhaps returning 0 in this case is better.Quoted 8 lines
The problem is that this code is also visited from
pnp_activate_dev()& co where this return value is needed. Forpnp_stop_dev()the same check (pnp_can_disable()) is performed in the suspend routine to avoid that particular problem. For resume my assumption was that a device that doesn't support activation will not have a driver attached to it. Perhaps this is wrong?Quoted 6 lines
i8042 registers drivers for keyboard and AUX ports to gather information whether the ports are present and what IRQ and IO ports shoudl be used to access them. And I have seen a few boxes that do not alloe [de]activate these devices.
Quoted 6 lines
But the activation is performed by the PNP layer when the driver is matched with a driver. So the driver isn't really responsible for the activation. It can prevent activation through
PNP_DRIVER_RES_DO_NOT_CHANGEthough, but that flag is also checked in the suspend routines.
You lost me... We have a scenario when a PNP driver is bound to a PNP device that does not support deactivation. Looking at the proposed PNP bus suspend code presence of such device will cause suspend process to fail. Are you saying this is what you want?
--
Dmitry
Quoted 6 lines · Dmitry Torokhov
You lost me... We have a scenario when a PNP driver is bound to a PNP device that does not support deactivation. Looking at the proposed PNP bus suspend code presence of such device will cause suspend process to fail. Are you saying this is what you want?
Ugh, scratch whatever I wrote earlier. Such devices should be marked with RES_DO_NOT_CHANEG so everything is fine.
Sorry about the noise.
--
Dmitry
Quoted 9 lines · Dmitry Torokhov
Quoted 6 lines · Dmitry Torokhov
You lost me... We have a scenario when a PNP driver is bound to a PNP device that does not support deactivation. Looking at the proposed PNP bus suspend code presence of such device will cause suspend process to fail. Are you saying this is what you want?
Quoted 4 lines
Ugh, scratch whatever I wrote earlier. Such devices should be marked with
RES_DO_NOT_CHANEGso everything is fine.Sorry about the noise.
So... You're OK with the patch as proposed?
Quoted 10 lines · Andrew Morton
Quoted 9 lines · Dmitry Torokhov
Quoted 6 lines · Dmitry Torokhov
You lost me... We have a scenario when a PNP driver is bound to a PNP device that does not support deactivation. Looking at the proposed PNP bus suspend code presence of such device will cause suspend process to fail. Are you saying this is what you want?
Quoted 7 lines
Quoted 4 lines
Ugh, scratch whatever I wrote earlier. Such devices should be marked with
RES_DO_NOT_CHANEGso everything is fine.Sorry about the noise.
So... You're OK with the patch as proposed?
Yes I am.
Signature · 1 line
Dmitry
Quoted 4 lines · Pierre Ossman
Add support for suspending devices connected to the PNP bus. New callbacks are added for the drivers and the PNP hardware layer is told to disable the device during the suspend.
The ALSA guys have gone off and implemented their own version of this, and
it's a bit different. I'll need to drop this patch now.
Please review http://www.zip.com.au/~akpm/linux/patches/stuff/git-alsa.patch, sort things out?
Quoted 9 lines · Andrew Morton
Quoted 5 lines · Pierre Ossman
Add support for suspending devices connected to the PNP bus. New callbacks are added for the drivers and the PNP hardware layer is told to disable the device during the suspend.
Quoted 7 lines
The ALSA guys have gone off and implemented their own version of this, and
it's a bit different. I'll need to drop this patch now.Please review http://www.zip.com.au/~akpm/linux/patches/stuff/git-alsa.patch, sort things out?
That things is huge! Do the ALSA guys perhaps have a patch with just the PnP bit in it?
Rgds
Pierre
Quoted 15 lines · Pierre Ossman
Quoted 12 lines · Andrew Morton
Quoted 5 lines · Pierre Ossman
Add support for suspending devices connected to the PNP bus. New callbacks are added for the drivers and the PNP hardware layer is told to disable the device during the suspend.
The ALSA guys have gone off and implemented their own version of this, and
it's a bit different. I'll need to drop this patch now.
Quoted 8 lines
Quoted 4 lines
Please review http://www.zip.com.au/~akpm/linux/patches/stuff/git-alsa.patch, sort things out?
That things is huge! Do the ALSA guys perhaps have a patch with just the PnP bit in it?
Quoted 28 lines · Andrew Morton
Quoted 25 lines · Pierre Ossman
Quoted 21 lines · Andrew Morton
Quoted 7 lines · Pierre Ossman
Add support for suspending devices connected to the PNP bus. New callbacks are added for the drivers and the PNP hardware layer is told to disable the device during the suspend.
The ALSA guys have gone off and implemented their own version of this, and
it's a bit different. I'll need to drop this patch now.Please review http://www.zip.com.au/~akpm/linux/patches/stuff/git-alsa.patch, sort things out?
Quoted 10 lines
Quoted 5 lines
That things is huge! Do the ALSA guys perhaps have a patch with just the PnP bit in it?
That patch is a bit dumber than mine. It doesn't do anything but call the driver supplied suspend/resume function, i.e. no PnP handling during suspend. It does handle cards though, something my patch doesn't do. Perhaps a combination of the two is acceptable to the ALSA crowd?
Rgds
Pierre
Quoted 10 lines · Pierre Ossman
That patch is a bit dumber than mine. It doesn't do anything but call the driver supplied suspend/resume function, i.e. no PnP handling during suspend. It does handle cards though, something my patch doesn't do. Perhaps a combination of the two is acceptable to the ALSA crowd?
Send an update agaisnt next -mm if you like. We can feed that in through the alsa tree too. It's not really appropriate to be updating the pnp system via the alsa tree, but as long as it's all in one place, it works.
Quoted 6 lines · Andrew Morton
Send an update agaisnt next -mm if you like. We can feed that in through the alsa tree too. It's not really appropriate to be updating the pnp system via the alsa tree, but as long as it's all in one place, it works.
Ok. I'll wait for your next compilation then.
Rgds
Pierre
Quoted 5 lines · Andrew Morton
Send an update agaisnt next -mm if you like. We can feed that in through the alsa tree too. It's not really appropriate to be updating the pnp system via the alsa tree, but as long as it's all in one place, it works.
Assuming the version you pointed out to me is the final one, then this patch will merge my changes into theirs.
Rgds
Pierre
[PNP] Improved PnP suspend support.
Also use the PnP functions to start/stop the devices during the suspend so that drivers will not have to duplicate this code.
---
drivers/pnp/driver.c | 37 ++++++++++++++++++++--- drivers/pnp/manager.c | 78 +++++++++++++++++++++++++++++++++++++------------ include/linux/pnp.h | 4 +++ 3 files changed, 95 insertions(+), 24 deletions(-)
drivers/pnp/driver.c +32 -5
@@ -150,19 +150,46 @@ static int pnp_bus_suspend(struct device {struct pnp_dev * pnp_dev =to_pnp_dev (dev);struct pnp_driver * pnp_drv = pnp_dev->driver; +int error; + +if (!pnp_drv) +return 0 ; + +if (pnp_drv->suspend) { + error = pnp_drv->suspend (pnp_dev, state); +if (error) +return error; + } + +if (!(pnp_drv->flags &PNP_DRIVER_RES_DO_NOT_CHANGE ) && +pnp_can_disable (pnp_dev)) { + error =pnp_stop_dev (pnp_dev); +if (error) +return error; + } -if (pnp_drv && pnp_drv->suspend) -return pnp_drv->suspend (pnp_dev, state);return 0 ; } -static void pnp_bus_resume (struct device *dev) +static int pnp_bus_resume (struct device *dev) {struct pnp_dev * pnp_dev =to_pnp_dev (dev);struct pnp_driver * pnp_drv = pnp_dev->driver; +int error; -if (pnp_drv && pnp_drv->resume) - pnp_drv->resume (pnp_dev); +if (!pnp_drv) +return 0 ; + +if (!(pnp_drv->flags &PNP_DRIVER_RES_DO_NOT_CHANGE )) { + error =pnp_start_dev (pnp_dev); +if (error) +return error; + } + +if (pnp_drv->resume) +return pnp_drv->resume (pnp_dev); + +return 0 ; }struct bus_type pnp_bus_type = {
drivers/pnp/manager.c +59 -19
118 lines not on this page · raw message
include/linux/pnp.h +4 -0
@@ -385,6 +385,8 @@ void pnp_init_resource_table(struct pnp_int pnp_manual_config_dev (struct pnp_dev *dev,struct pnp_resource_table *res,int mode);int pnp_auto_config_dev (struct pnp_dev *dev);int pnp_validate_config (struct pnp_dev *dev); +int pnp_start_dev (struct pnp_dev *dev); +int pnp_stop_dev (struct pnp_dev *dev);int pnp_activate_dev (struct pnp_dev *dev);int pnp_disable_dev (struct pnp_dev *dev);void pnp_resource_change (struct resource *resource,unsigned long start,unsigned long size);
@@ -428,6 +430,8 @@ static inline void pnp_init_resource_tabstatic inline int pnp_manual_config_dev (struct pnp_dev *dev,struct pnp_resource_table *res,int mode) {return -ENODEV ; }static inline int pnp_auto_config_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline int pnp_validate_config (struct pnp_dev *dev) {return -ENODEV ; } +static inline int pnp_start_dev (struct pnp_dev *dev) {return -ENODEV ; } +static inline int pnp_stop_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline int pnp_activate_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline int pnp_disable_dev (struct pnp_dev *dev) {return -ENODEV ; }static inline void pnp_resource_change (struct resource *resource,unsigned long start,unsigned long size) { }
At Tue, 29 Nov 2005 20:49:44 +0100,
Quoted 42 lines · Pierre Ossman
Quoted 28 lines · Andrew Morton
Quoted 25 lines · Pierre Ossman
Quoted 21 lines · Andrew Morton
Quoted 7 lines · Pierre Ossman
Add support for suspending devices connected to the PNP bus. New callbacks are added for the drivers and the PNP hardware layer is told to disable the device during the suspend.
The ALSA guys have gone off and implemented their own version of this, and
it's a bit different. I'll need to drop this patch now.Please review http://www.zip.com.au/~akpm/linux/patches/stuff/git-alsa.patch, sort things out?
Quoted 10 lines
Quoted 5 lines
That things is huge! Do the ALSA guys perhaps have a patch with just the PnP bit in it?
Quoted 4 lines
That patch is a bit dumber than mine. It doesn't do anything but call the driver supplied suspend/resume function, i.e. no PnP handling during suspend. It does handle cards though, something my patch doesn't do. Perhaps a combination of the two is acceptable to the ALSA crowd?
Yep, my implementation is only for very basic things and may lack of many stuff. Any fix would be welcome.
Takashi