[Pm-utils] powerpc support

[Stefan Seyfried]

On Tue, Mar 27, 2007 at 11:36:16AM +0200, Tim Dijkstra wrote:
> Hi,
> 
> This is a first cut at supporting powerpc for s2*. For the s2ram bit it
> basically removes the vbetool code. This has two reasons. First, there
> isn't a powerpc version of libx86 yet. Second, the machines that have
> support for s2ram in the kernel don't need those hacks.
> 
> As you noticed, I've asked some people to test it a bit already. The
> s2ram part seems to work fine, the s2disk part still has some problems,
> which could be related to its BE nature. But this will have to be fixed
> anyway.
> 
> With this patch people can at least compile and give us some feedback.

ok, some comments inlined.

>  int s2ram_prepare(void)
>  {
> +#ifdef CONFIG_PPC
> +	return 0;
> +#else
>  	int ret, id;
>  
>  	dmi_scan();

how about:

#ifdef CONFIG_PPC
#define s2ram_prepare() 0
#else
int s2ram_prepare(void)
{
...
}
#endif

This lets the compiler optimize this stuff out very well.

> +#ifdef CONFIG_PPC
> +/* This will be deprecated, but is needed on older kernels (< May 2006) */ 
> +int s2ram_do_pmu(void) {
> +        int ret = 0,  fd;
> +        unsigned long arg = 0;
> +
> +        if ((fd = open("/dev/pmu", O_RDWR)) < 0) 
> +                return errno;
> +        
> +        ret = ioctl(fd, PMU_IOC_CAN_SLEEP, &arg);
> +
> +        if (!ret && arg != 1) 
> +                ret = ENOTSUP;
> +
> +        if (!ret)
> +                ret = ioctl(fd, PMU_IOC_SLEEP, arg);
> +
> +        close(fd);
> +
> +	return ret;
>  }

#else
#define s2ram_do_pmu() ENODEV


> +#endif
>  
>  /* Actually enter the suspend. May be ran on frozen system. */
>  int s2ram_do(void)
>  {
> -	int ret = 0;
> -	FILE *f = fopen("/sys/power/state", "w");
> +       int ret = 0;
> +	FILE *f;
> +
> +#ifdef CONFIG_PPC

and get rid of CONFIG_PPC here.

> +	ret = s2ram_do_pmu();
> +	/* If the PMU says not supported, we better stop here, we could
> +	 * crash the machine on some kernels. On success we return too;) */
> +	if (!ret || (ret == ENOTSUP))
> +		return ret;
> +	/* Else we just continue as if nothing happened, future kernels
> +	 * will work with /s/p/s. */
> +	ret = 0;
> +#endif
> +	f = fopen("/sys/power/state", "w");
>  	if (!f) {
>  		printf("/sys/power/state does not exist; what kind of ninja mutant machine is this?\n");
>  		return ENODEV;
> @@ -260,6 +322,7 @@
>  } 
>  
>  void s2ram_resume(void)
>  {
> +#ifndef CONFIG_PPC
>  	if (flags & PCI_SAVE) {
>  		printf("restoring PCI config of device %02x:%02x.%d\n",
>  			vga_dev.bus, vga_dev.dev, vga_dev.func);
> @@ -297,6 +351,7 @@
>  		printf("Calling radeon_cmd_light(1)\n");
>  		radeon_cmd_light(1);
>  	}
> +#endif
>  }
>  
>  #ifndef CONFIG_BOTH
> @@ -306,6 +361,7 @@
>  	       "\n"
>  	       "Options:\n"
>  	       "    -h, --help:       this text.\n"
> +#ifndef CONFIG_PPC
>  	       "    -n, --test:       test if the machine is in the database.\n"
>  	       "                      returns 0 if known and supported\n"
>  	       "    -i, --identify:   prints a string that identifies the machine.\n"
> @@ -322,6 +378,7 @@
>  				       "suspend\n"
>  	       "                      1=s3_bios, 2=s3_mode, 3=both\n"
>  	       "    -v, --pci_save:   save the PCI config space for the VGA card.\n"
> +#endif
>  	       "\n");
>  	exit(1);
>  }
> @@ -331,8 +388,8 @@
>  	int i, id = -1, ret = 0, test_mode = 0, force = 0;
>  	int active_console = -1;
>  	struct option options[] = {
> -		{ "test",	no_argument,		NULL, 'n'},
>  		{ "help",	no_argument,		NULL, 'h'},
> +		{ "test",	no_argument,		NULL, 'n'},
>  		{ "force",	no_argument,		NULL, 'f'},
>  		{ "vbe_save",	no_argument,		NULL, 's'},
>  		{ "vbe_post",	no_argument,		NULL, 'p'},
> @@ -349,6 +406,7 @@
>  		case 'h':
>  			usage();
>  			break;
> +#ifndef CONFIG_PPC
>  		case 'i':
>  			dmi_scan();
>  			identify_machine();
> @@ -377,11 +435,21 @@
>  		case 'v':
>  			flags |= PCI_SAVE;
>  			break;
> +#else 
> +		/* For consistency with the non-ppc version, it seems best to 
> +		 * me to accept, but ignore the s2ram 'hacks'. */

I don't think so. Uper layer passing random options to programs should be
fixed. We should not let them go away with it ;-)

>  		default:
> +			fprintf(stderr, "The powerpc version of s2ram doesn't "
> +					"take any options.\n");
> +			break;
> +#endif
> +		case '?':

Will probably not work, since it is not in options[] above?

> +#ifndef CONFIG_PPC
>  	if (flags && !force) {
>  		printf("acpi_sleep, vbe_save, vbe_post and radeontool parameter "
>  		       "must be used with --force\n\n");
> @@ -414,15 +482,17 @@
>  
>  	if (ret)
>  		goto out;
> -
> +#endif
>  	/* switch to console 1 first, since we might be in X */
>  	active_console = fgconsole();
>  	printf("Switching from vt%d to vt1\n", active_console);
>  	chvt(1);
>  
> +#ifndef CONFIG_PPC
>  	ret = s2ram_hacks();

#ifdef CONFIG_PPC
#define s2ram_hacks() 0
#else
int s2ram_hacks()
...
#endif

above, so we can again get rid of the CONFIG_PPC here.

>  	if (ret)
>  		goto out;
> +#endif
>  	ret = s2ram_do();
>  	s2ram_resume();

We need CONFIG_PPC, but i personally prefer it to be in blocks somewhere
outside the main code flow and not sprinkled all over it ;-)

But this is just my personal preference, and i am not claiming any authority
on that ;-)

Basically it looks good to me.
-- 
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out."