On Tuesday 01 October 2013 10:20 AM, Michael Ellerman wrote:
> On Mon, Sep 30, 2013 at 04:47:29PM +0530, Madhavan Srinivasan wrote:
>> Currently pmc setup macros are used for non pmc sprs. This patch
>> add new set of macros and cleans up the code to use the new setup macro
>> for non pmc sprs.
>
> Hi Maddy,
>
> Firstly you should use "PMC" not pmc, it's an acronym. You should also
> spell out what it means the first time you use it, eg:
>
> Currently the PMC (Performance Monitor Counter) macros are used ..
>
> Secondly you need to say _why_ it is a bad idea to use the PMC macros
> for non-PMC SPRs.
>
Will change it.
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 27a90b9..73b6f9f 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -139,6 +139,37 @@ static ssize_t __used \
>> return count; \
>> }
>>
>> +#define SYSFS_SPRSETUP(NAME, ADDRESS) \
>> +static void read_##NAME(void *val) \
>> +{ \
>> + *(unsigned long *)val = mfspr(ADDRESS); \
>> +} \
>> +static void write_##NAME(void *val) \
>> +{ \
>> + mtspr(ADDRESS, *(unsigned long *)val); \
>> +} \
>> +static ssize_t show_##NAME(struct device *dev, \
>> + struct device_attribute *attr, \
>> + char *buf) \
>> +{ \
>> + struct cpu *cpu = container_of(dev, struct cpu, dev); \
>> + unsigned long val; \
>> + smp_call_function_single(cpu->dev.id, read_##NAME, &val, 1); \
>> + return sprintf(buf, "%lx\n", val); \
>> +} \
>> +static ssize_t __used \
>> + store_##NAME(struct device *dev, struct device_attribute *attr, \
>> + const char *buf, size_t count) \
>> +{ \
>> + struct cpu *cpu = container_of(dev, struct cpu, dev); \
>> + unsigned long val; \
>> + int ret = sscanf(buf, "%lx", &val); \
>> + if (ret != 1) \
>> + return -EINVAL; \
>> + smp_call_function_single(cpu->dev.id, write_##NAME, &val, 1); \
>> + return count; \
>> +}
>
> This is basically a complete copy of the SYSFS_PMCSETUP() macro, except
> for the one line removal of the call to ppc_enable_pmcs().
>
> You should be able to do better, by defining a macro that does all the
> boiler plate and takes an "extra" argument, which for PMCs is
> "ppc_enable_pmcs()" and for regular SPRs is empty.
>
Sorry about that. Will make the changes.
> cheers
>
Thanks for the feedback
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
