Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932146AbdDDWLb (ORCPT ); Tue, 4 Apr 2017 18:11:31 -0400 Received: from mga04.intel.com ([192.55.52.120]:14204 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752997AbdDDWL3 (ORCPT ); Tue, 4 Apr 2017 18:11:29 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,276,1486454400"; d="scan'208";a="84378706" Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com Subject: Re: [PATCH v5 2/6] platform/x86: intel_pmc_ipc: Add pmc gcr read/write/update api's References: <83d48f6907b3392e38ee251cf9fa6d518f139288.1491264643.git.sathyanarayanan.kuppuswamy@linux.intel.com> To: Andy Shevchenko Cc: Andy Shevchenko , Zha Qipeng , "dvhart@infradead.org" , Guenter Roeck , Wim Van Sebroeck , Sathyanarayanan Kuppuswamy Natarajan , David Box , Rajneesh Bhardwaj , Platform Driver , "linux-kernel@vger.kernel.org" , linux-watchdog@vger.kernel.org From: sathyanarayanan kuppuswamy Organization: Intel Message-ID: Date: Tue, 4 Apr 2017 15:07:32 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2853 Lines: 87 Hi Andy, On 04/04/2017 06:53 AM, Andy Shevchenko wrote: > On Tue, Apr 4, 2017 at 3:24 AM, Kuppuswamy Sathyanarayanan > 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