Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753003AbcJKL31 (ORCPT ); Tue, 11 Oct 2016 07:29:27 -0400 Received: from mga05.intel.com ([192.55.52.43]:43893 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752894AbcJKL3Z (ORCPT ); Tue, 11 Oct 2016 07:29:25 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,329,1473145200"; d="scan'208";a="18389229" Date: Tue, 11 Oct 2016 14:29:16 +0300 From: Jarkko Sakkinen To: "Winkler, Tomas" Cc: Peter Huewe , "moderated list:TPM DEVICE DRIVER" , open list Subject: Re: [tpmdd-devel] [PATCH 2/3] tpm_crb: encapsulate crb_wait_for_reg_32 Message-ID: <20161011112916.GD21688@intel.com> References: <1476177787-15003-1-git-send-email-jarkko.sakkinen@linux.intel.com> <1476177787-15003-4-git-send-email-jarkko.sakkinen@linux.intel.com> <5B8DA87D05A7694D9FA63FD143655C1B542F7FD8@hasmsx108.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B542F7FD8@hasmsx108.ger.corp.intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1679 Lines: 48 On Tue, Oct 11, 2016 at 10:21:03AM +0000, Winkler, Tomas wrote: > > Encapsulated crb_wait_for_reg32() so that state changes in other CRB registers > > than TPM_CRB_CTRL_REQ_x can be waited. > > > > Signed-off-by: Jarkko Sakkinen > > --- > > drivers/char/tpm/tpm_crb.c | 40 +++++++++++++++++++++++----------------- > > 1 file changed, 23 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > > c34318b..45f53c2 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -121,6 +121,25 @@ static int __maybe_unused crb_go_idle(struct device > > *dev, struct crb_priv *priv) > > return 0; > > } > > > > +static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, > > + unsigned long timeout) > This is a boiler plate register polling function I would call it _poll_ rather the _wait_ > > > +{ > > + ktime_t start; > > + ktime_t stop; > > + > > + start = ktime_get(); > > + stop = ktime_add(start, ms_to_ktime(timeout)); > > + > > + do { > > + if ((ioread32(reg) & mask) == value) > I prefer the register value is synced to variable, this inlining is > harder to add adhoc debug prints. Also you removed the debug print > out that I know when this settled which is important for catching > bugs. I can add it but can you just briefly explain why the warning is not enough? > > + return true; > > + > > + usleep_range(50, 100); > How do you know this is correct sleep time, I've tuned that for power > gating I'm not sure you this fits also for locality. Does it matter as long as it is less than for the timeout? /Jarkko