[PATCH v3] powerpc/powernv: add hdat attribute to sysfs

[Michael Ellerman]

Andrew Donnellan <andrew.donnellan at au1.ibm.com> writes:

> On 24/02/17 17:20, Matt Brown wrote:
>> The HDAT data area is consumed by skiboot and turned into a device-tree.
>> In some cases we would like to look directly at the HDAT, so this patch
>> adds a sysfs node to allow it to be viewed.  This is not possible through
>> /dev/mem as it is reserved memory which is stopped by the /dev/mem filter.
>>
>> Signed-off-by: Matt Brown <matthew.brown.dev at gmail.com>
>
> Changes look good, thanks for addressing the comments! Still a couple of 
> minor points below, otherwise:
>
> Reviewed-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
>
> Stewart: this might need your ACK?

I don't see it in skiboot yet ?

>> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
>> index b5d98cb..9a0c9d6 100644
>> --- a/arch/powerpc/platforms/powernv/Makefile
>> +++ b/arch/powerpc/platforms/powernv/Makefile
>> @@ -3,6 +3,7 @@ obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
>>  obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
>>  obj-y			+= opal-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
>>  obj-y			+= opal-kmsg.o
>> +obj-y			+= opal-hdat.o
>
> Normally we keep putting new object files on the same line until it gets 
> long enough that we have to break it. This is very minor though :)

Yeah, please put it on the same line as opal-kmsg.o

>> diff --git a/arch/powerpc/platforms/powernv/opal-hdat.c b/arch/powerpc/platforms/powernv/opal-hdat.c
>> new file mode 100644
>> index 0000000..3315dd3
>> --- /dev/null
>> +++ b/arch/powerpc/platforms/powernv/opal-hdat.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * PowerNV OPAL HDAT interface
>> + *
>> + * Author: Matt Brown <matthew.brown.dev at gmail.com>
>> + *
>> + * Copyright 2017 IBM Corp.

I dislike email addresses in source files, they just end up being wrong
and needing to be updated. We have your email in the commit log anyway.

So I prefer:

 * Copyright 2017, Matt Brown, IBM Corp.

Which I believe is also blessed by the lawyers.

>> + * 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 of the License, or (at your option) any later version.
>> + */
>> +
>> +#include <asm/io.h>
>> +#include <asm/opal.h>
>> +#include <linux/of.h>
>> +#include <linux/types.h>

Preferred style is to put the linux includes first, before the asm ones.

>> +struct hdat_info {
>> +	char *base;
>> +	u64 size;
>> +};
>> +
>> +static struct hdat_info hdat_inf;
>
> As Oliver pointed out, we could do with a better name than hdat_inf - 
> it's only one character away from the name of the struct type. Hmm, 
> perhaps "hdat_location", or maybe Oliver has a better suggestion.

Why not hdat_info ?

In fact for bonus points, you can just do:

static struct {
	char *base;
	u64 size;
} hdat_info;

cheers