Re: Powerop-core.patch

[Pavel Machek <pavel@suse.cz>]

Hi!

> Here is the core patch for the PowerOp concept.  It adds the powerop  
> struct
> for opertaing point support to linux/pm.h and adds support to 
> transition to supported operating points by
> setting their name into /sys/power/state.
> 
> The supported operating points are shown in a readonly sysfs file,
> /sys/power/supported_states.

Could you fix the /sys interface to be value-per-file, and split it to
separate patch?

> -static const char * const pm_states[PM_SUSPEND_MAX] = {
> -       [PM_SUSPEND_STANDBY]    = "standby",
> -       [PM_SUSPEND_MEM]        = "mem",
> +static struct powerop standby = {
> +       .name = "standby",
> +       .description = "Power-On Suspend ACPI State: S1",
> +       .type = PM_SUSPEND_STANDBY,
> +};

You got the description fields wrong...

> +static struct powerop mem = {
> +       .name = "mem   ",
> +       .description = "Suspend-to-RAM ACPI State: S3",
> +       .type = PM_SUSPEND_MEM,
> +};

...for example mem is used on non-acpi machines, too. Plus, it is
useless.

> +#endif
> 
> -static inline int valid_state(suspend_state_t state)
> +/*
> + *
> + */
> +static int pm_change_state(struct powerop *state)

Eh?

> +       /*
> +        * list_find new operating point.
> +        * compare to current operating point.
> +        * if different change to new operating point.
> +        */
> +       list_for_each_entry_safe(this, next, head, list) {
> +               if (strncmp(state->name, this->name, len) == 0) {
> +                       if ((strcmp(current_state->name, this->name)) 
> == 0) {

Do we really need to do list search here?

> +                               return 0;
> +                       }

And please avoid {} around single return.

> +                       /*
> +                        * now lets wait for the transition latency
> +                        */
> +                       udelay(this->latency);

udelay in generic code seems wrong.

> @@ -211,7 +275,15 @@ static int enter_state(suspend_state_t s
>    */
>   int software_suspend(void)
>   {
> -       return enter_state(PM_SUSPEND_DISK);
> +       struct powerop *this, *next;
> +       struct list_head *head = &pm_states.list;
> +       int error = 0;
> +
> +       list_for_each_entry_safe(this, next, head, list) {
> +               if (this->type == PM_SUSPEND_DISK)
> +                       error= enter_state(this);
> +       }
> +       return error;
>   }

Ugly....

> +struct powerop {
> +       struct list_head list;
> +       suspend_state_t type;
> +       char name[PM_NAME_SIZE];
> +       char description[PM_DESCRIPTION_SIZE];
> +       unsigned int frequency;         /* in KHz */
> +       unsigned int voltage;           /* mV */

having voltage/frequency/latency field for suspend-to-disk seems wrong.

-- 
Thanks for all the (sleeping) penguins.