Re: [patch] enums to clear suspend-state confusion

[Andrew Morton <akpm@osdl.org>]

Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
> 
> > This patch should clear up some confusion between driver model and
> > drivers people, and also prepares way to add runtime power managment
> > later. Please apply,
> 
> PCI states are now marked with PCI_D? so that confusion is not
> possible. Andrew complained about warning in to_pci_state(); I do not
> see it, but I added catch-all return anyway.
> 
> I'd like this to be applied, so I can start fixing the drivers...
> 

Sure, let's try to get this done.

> +static inline enum pci_state to_pci_state(suspend_state_t state)
> +{
> +	if (SUSPEND_EQ(state, PM_SUSPEND_ON))
> +		return PCI_D0;
> +	if (SUSPEND_EQ(state, PM_SUSPEND_STANDBY))
> +		return PCI_D1;
> +	if (SUSPEND_EQ(state, PM_SUSPEND_MEM))
> +		return PCI_D3hot;
> +	if (SUSPEND_EQ(state, PM_SUSPEND_DISK))
> +		return PCI_D3cold;
> +	BUG();
> +	return PCI_D0;	/* akpm complained about warnings? */
> +}
> +
> ...
> +/*
> + * For now, drivers only get system state. Later, this is going to become
> + * structure or something to enable runtime power managment.
> + */
> +typedef enum system_state suspend_state_t;
> +
> +#define SUSPEND_EQ(a, b) (a == b)
> +
>  enum {
>  	PM_DISK_FIRMWARE = 1,
>  	PM_DISK_PLATFORM,

This is a bit ugly, and I don't think it actually works.

If, at some time in the future you change the suspend state to a struct
then you will want to pass that thing around by reference, not by value. 
Hence your new suspend_state_t will need to be typecast to a pointer to
struct, and not a struct.  And that's not a thing which we do in-kernel
much at all.  (There's nothing wrong with the practice per-se, but in the
kernel it does violate the principle of least surprise).

So if you really do intend to add more things to the suspend state I'd
suggest that you set the final framework in place immediately.  Do:

struct suspend_state {
	enum system_state state;
}

static inline enum pci_state to_pci_state(struct suspend_state *state)
{
	if (state->state == PM_SUSPEND_ON)
		return PCI_D0;
	if (state->state == PM_SUSPEND_STANDBY)
		return PCI_D1;
	...
}