Perhaps Mr Morton can have a look? This patch is probably -mm material
anyhow.
Rgds
Pierre
On 11/4/05, Pierre Ossman <[email protected]> wrote:
> +
> +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.
> + 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.
> +
> +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
Dmitry Torokhov wrote:
> On 11/4/05, Pierre Ossman <[email protected]> wrote:
>
>> +
>> +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. :)
>> + 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.
>
>
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
On 11/4/05, Pierre Ossman <[email protected]> wrote:
> Dmitry Torokhov wrote:
> >
> > 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.
> >
> >
>
> 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?
>
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
Dmitry Torokhov wrote:
> On 11/4/05, Pierre Ossman <[email protected]> wrote:
>
>> Dmitry Torokhov wrote:
>>
>>> 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.
>>>
>>>
>>>
>> 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?
>>
>>
>
> 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
On 11/4/05, Pierre Ossman <[email protected]> wrote:
> Dmitry Torokhov wrote:
> > On 11/4/05, Pierre Ossman <[email protected]> wrote:
> >
> >> Dmitry Torokhov wrote:
> >>
> >>> 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.
> >>>
> >>>
> >>>
> >> 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?
> >>
> >>
> >
> > 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.
>
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
On 11/4/05, Dmitry Torokhov <[email protected]> wrote:
>
> 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
Dmitry Torokhov <[email protected]> wrote:
>
> On 11/4/05, Dmitry Torokhov <[email protected]> wrote:
> >
> > 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.
So... You're OK with the patch as proposed?
On Saturday 05 November 2005 02:15, Andrew Morton wrote:
> Dmitry Torokhov <[email protected]> wrote:
> >
> > On 11/4/05, Dmitry Torokhov <[email protected]> wrote:
> > >
> > > 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.
>
> So... You're OK with the patch as proposed?
>
Yes I am.
--
Dmitry
Pierre Ossman <[email protected]> wrote:
>
> 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?
Andrew Morton wrote:
>Pierre Ossman <[email protected]> wrote:
>
>
>>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?
>
>
That things is huge! Do the ALSA guys perhaps have a patch with just the
PnP bit in it?
Rgds
Pierre
Pierre Ossman <[email protected]> wrote:
>
> Andrew Morton wrote:
>
> >Pierre Ossman <[email protected]> wrote:
> >
> >
> >>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?
> >
> >
>
> That things is huge! Do the ALSA guys perhaps have a patch with just the
> PnP bit in it?
>
http://www.kernel.org/git/gitweb.cgi?p=linux/kernel/git/perex/alsa-current.git;a=commitdiff;h=5353d906effe648dd20899fe61ecb6982ad93cdd
Andrew Morton wrote:
>Pierre Ossman <[email protected]> wrote:
>
>
>>Andrew Morton wrote:
>>
>>
>>
>>>Pierre Ossman <[email protected]> wrote:
>>>
>>>
>>>
>>>
>>>>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?
>>>
>>>
>>>
>>>
>>That things is huge! Do the ALSA guys perhaps have a patch with just the
>>PnP bit in it?
>>
>>
>>
>
>http://www.kernel.org/git/gitweb.cgi?p=linux/kernel/git/perex/alsa-current.git;a=commitdiff;h=5353d906effe648dd20899fe61ecb6982ad93cdd
>
>
>
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
Pierre Ossman <[email protected]> wrote:
>
> >http://www.kernel.org/git/gitweb.cgi?p=linux/kernel/git/perex/alsa-current.git;a=commitdiff;h=5353d906effe648dd20899fe61ecb6982ad93cdd
> >
> >
> >
>
> 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.
Andrew Morton wrote:
>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
Andrew Morton wrote:
> 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
At Tue, 29 Nov 2005 20:49:44 +0100,
Pierre Ossman wrote:
>
> Andrew Morton wrote:
>
> >Pierre Ossman <[email protected]> wrote:
> >
> >
> >>Andrew Morton wrote:
> >>
> >>
> >>
> >>>Pierre Ossman <[email protected]> wrote:
> >>>
> >>>
> >>>
> >>>
> >>>>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?
> >>>
> >>>
> >>>
> >>>
> >>That things is huge! Do the ALSA guys perhaps have a patch with just the
> >>PnP bit in it?
> >>
> >>
> >>
> >
> >http://www.kernel.org/git/gitweb.cgi?p=linux/kernel/git/perex/alsa-current.git;a=commitdiff;h=5353d906effe648dd20899fe61ecb6982ad93cdd
> >
> >
> >
>
> 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