Hehe, i replied on the wrong list :-)
> 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."
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Suspend-devel mailing list
Suspend-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/suspend-devel
