Re: [PATCH v5 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's

[sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

Hi Andy,


On 04/04/2017 06:53 AM, Andy Shevchenko wrote:
> On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> This patch adds API's to read/write/update PMC GC registers.
>> PMC dependent devices like iTCO_wdt, Telemetry has requirement
>> to acces GCR registers. These API's can be used for this
>> purpose.
>> +/* GCR reg offsets from gcr base*/
>> +#define PMC_GCR_PMC_CFG_REG            0x08
>> +
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -127,6 +127,7 @@ static struct intel_pmc_ipc_dev {
>>
>>          /* gcr */
>>          resource_size_t gcr_base;
>> +       void __iomem *gcr_mem_base;
>>          int gcr_size;
> Rearrange those lines to make __iomem pointer latter.
Will do it in next version. But the gcr_base resource pointer will be 
removed in next dependent patch. So I don't think it matters in the end.
>
>> +static inline int is_gcr_valid(u32 offset)
> Same comment as previously. It should take a pointer to struct
> intel_pmc_ipc_dev as a parameter.
This needs the driver cleanup. We can do it in refactoring patch.
>
>> +/**
>> + * intel_pmc_gcr_write() - Write PMC GCR register
>> + * @offset:    offset of GCR register from GCR address base
>> + * @data:      register update value
>> + *
>> + * Writes the PMC GCR register of given offset with given
>> + * value
> You have to use proper punctuation in the sentences in full Description section.
Will fix it in next version.
>
>> +/**
>> + * intel_pmc_gcr_update() - Update PMC GCR register bits
>> + * @offset:    offset of GCR register from GCR address base
>> + * @mask:      bit mask for update operation
>> + * @val:       update value
>> + *
>> + * Updates the bits of given GCR register as specified by
>> + * @mask and @val
> Ditto.
>
>> +int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>> +{
>> +       u32 orig, tmp;
> One of them is redundant.
>
> I would go just with
> u32 value;
Will fix it in next version.
>
>> +       writel(tmp, ipcdev.gcr_mem_base + offset);
>> +
>> +       tmp = readl(ipcdev.gcr_mem_base + offset);
>> +
> Redundant.
>
>> +       if ((tmp & mask) != (val & mask)) {
> Why?! It the a case when writel() will fail? Needs to be commented.
Yes, I am checking whether the update is successful or  not. I can add a 
comment there to explain it.
>
>> +               ret = -EIO;
>> +               goto gcr_update_err;
>> +       }
>> +
>> +gcr_update_err:
> The keyword  'unlock' is missed in the label name.
"goto" to this label is only used when there is an error in update 
operation. Do you think we should still rename it to gcr_ipc_unlock ?
>
>> +       mutex_unlock(&ipclock);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer