Re: [patch] pm: fix runtime powermanagement's /sys interface

[Pavel Machek <pavel@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 2388 bytes --]

On Čt 05-01-06 17:37:30, Patrick Mochel wrote:
> 
> 
> On Fri, 6 Jan 2006, Pavel Machek wrote:
> 
> > On ÄŒt 05-01-06 16:04:07, Patrick Mochel wrote:
> 
> > > A better point, and one that would actually be useful, would be to remove
> > > the file altogether. Let Dominik export a power file, with complete
> > > control over the values, for each pcmcia device. Then you never have to
> > > worry about breaking PCMCIA again.
> >
> > Fine with me.
> 
> ACK, you beat me to it.
> 
> And, appended is a patch to export PM controls for PCI devices. The file
> "pm_possible_states" exports the states a device supports, and "pm_state"
> exports the current state (and provides the interface for entering a
> state).
> 
> Eventually, some drivers will want to fix up those values so that it can
> mask of states that it doesn't support, as well as offer possible device-
> specific states.
> 
> What's interesting is that with this patch, I can see that two more
> devices on my system support D1 and D2 -- the cardbus controllers, which
> are actually bridges whose PM capabilities aren't exported via
lspci.

> +static ssize_t pm_possible_states_show(struct device * d,
> +				       struct device_attribute * a,
> +				       char * buffer)
> +{
> +	struct pci_dev * dev = to_pci_dev(d);
> +	char * s = buffer;
> +
> +	s += sprintf(s, "d0");
> +	if (dev->poss_states[PCI_D1])
> +		s += sprintf(s, " d1");
> +	if (dev->poss_states[PCI_D2])
> +		s += sprintf(s, " d2");
> +	if (dev->poss_states[PCI_D3hot])
> +		s += sprintf(s, " d3");

Could we use same states as rest of code, i.e. "D2" instead of "d2"
and "D3hot" instead of "d3"?

> +static ssize_t pm_state_show(struct device * d, struct device_attribute * a,
> +			     char * buffer)

Please no space between "struct foo *" and "d".

> +	if (state == dev->current_state)
> +		return 0;
> +
> +	if (dev->poss_states[state])
> +		ret = pci_set_power_state(dev, state);

So you just set the PCI power, without any coordination with driver?
How can this work?

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -106,6 +106,7 @@ struct pci_dev {
>  					   this if your device has broken DMA
>  					   or supports 64-bit transfers.  */
> 
> +	u32		poss_states[4];

It is boolean... Can we have int instead of u32?

								Pavel

-- 
Thanks, Sharp!

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]