Re: [3/3,v3] powerpc/powernv: Add opal-prd channel

[Jeremy Kerr <jk@ozlabs.org>]

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!

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

>> + *
>> + * Author: Vaidyanathan Srinivasan <svaidy at linux.vnet.ibm.com>
>> + * Author: Jeremy Kerr <jk@ozlabs.org>
> 
> 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.

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

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

>> +	switch(cmd) {
>               ^
> 	      space please

Fixed.


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

>> +struct file_operations opal_prd_fops = {
> 
> This can be static const I think.

Indeed it can! Updated.

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