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

[Pavel Machek <pavel@ucw.cz>]

Hi!


> > 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.

I agree about the ugly bit :-(.

> 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. 

Actually I expect it to become struct of two members, system-state and
bus-specific state. That seems small enough to pass 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;
> }

I can do that... but it will break compilation of every driver in the
tree. I can fix drivers I use and try to fix some more will sed, but
it will be painfull (and pretty big diff, and I'll probably miss some).

Should I do that?
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!