Hi Michael,

Sorry, I put this in but then hit the build break, I was going to fix it up but
would rather you did and tested it, so we may as well do another review :)

whee!

quoted

@@ -0,0 +1,58 @@
+/*
+ * OPAL Runtime Diagnostics interface driver
+ * Supported on POWERNV platform
+ *
+ * (C) Copyright IBM 2015

Usual syntax is: "Copyright IBM Corporation 2015"

OK, fixed.

quoted

+ *
+ * Author: Vaidyanathan Srinivasan <svaidy at linux.vnet.ibm.com>
+ * Author: Jeremy Kerr [off-list ref]

I'd rather you dropped these, they'll just bit rot, but if you insist I don't
care that much.

Yep, I'd rather remove them too.

quoted

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.

As pointed out by Daniel, we should probably be using the "version 2" only
language on new files.

Fixed.

quoted

+	vma->vm_page_prot = phys_mem_access_prot(file, vma->vm_pgoff,
+						 size, vma->vm_page_prot)
+				| _PAGE_SPECIAL;

This doesn't build with CONFIG_STRICT_MM_TYPECHECKS=y:

  arch/powerpc/platforms/powernv/opal-prd.c:131:5: error: invalid operands to binary | (have ‘pgprot_t’ and ‘int’)
  | _PAGE_SPECIAL;

OK, new patch coming with the proper pgprot macros.

quoted

+	switch(cmd) {

              ^
	      space please

Fixed.

quoted

+		pr_devel("ioctl SCOM_READ: chip %llx addr %016llx "
+				"data %016llx rc %lld\n",

Don't split the string please.

OK, but this makes our lines >80 chars. Assuming that'll be okay.

quoted

+struct file_operations opal_prd_fops = {

This can be static const I think.

Indeed it can! Updated.

quoted

+static struct miscdevice opal_prd_dev = {
+        .minor		= MISC_DYNAMIC_MINOR,
+        .name		= "opal-prd",
+        .fops		= &opal_prd_fops,

White space is messed up here, should be leading tabs.

[tabs-spaces-both.png]

Thanks for the review, new patch coming soon.

Cheers,


Jeremy