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 <jarkko.sakki...@linux.intel.com> > > --- > > 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