From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753353AbdDND60 (ORCPT ); Thu, 13 Apr 2017 23:58:26 -0400 Received: from mailgw01.mediatek.com ([210.61.82.183]:49951 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751544AbdDND6X (ORCPT ); Thu, 13 Apr 2017 23:58:23 -0400 Message-ID: <1492142296.6147.19.camel@mtkswgap22> Subject: Re: [PATCH 2/2] hwrng: mtk: Add driver for hardware random generator on MT7623 SoC From: Sean Wang To: PrasannaKumar Muralidharan CC: Herbert Xu , Matt Mackall , Rob Herring , Mark Rutland , Corentin LABBE , "Romain Perier" , , "Wei Yongjun" , , , , , , Date: Fri, 14 Apr 2017 11:58:16 +0800 In-Reply-To: References: <1492067108-14748-1-git-send-email-sean.wang@mediatek.com> <1492067108-14748-3-git-send-email-sean.wang@mediatek.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi PrasannaKumar, Add my comments inline On Thu, 2017-04-13 at 14:09 +0530, PrasannaKumar Muralidharan wrote: > Hi Sean, > > Mostly looks good, have few minor comments. > > On 13 April 2017 at 12:35, wrote: > > +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait) > > +{ > > + struct mtk_rng *priv = to_mtk_rng(rng); > > + int ready; > > + > > + ready = readl(priv->base + RNG_CTRL) & RNG_READY; > > + if (!ready && wait) > > + readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready, > > + ready & RNG_READY, USEC_POLL, > > + TIMEOUT_POLL); > > + return !!ready; > > +} > > Use readl_poll_timeout_atomic's return value or -EIO instead of > !!ready. This will simplify mtk_rng_read. > !!ready provided is in order to let blocking/non-blocking case could share same code path. And readl_poll_timeout_atomic only handles blocking case. > > +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) > > +{ > > + struct mtk_rng *priv = to_mtk_rng(rng); > > + int retval = 0; > > + > > + while (max >= sizeof(u32)) { > > + if (!mtk_rng_wait_ready(rng, wait)) > > + break; > > + > > + *(u32 *)buf = readl(priv->base + RNG_DATA); > > + retval += sizeof(u32); > > + buf += sizeof(u32); > > + max -= sizeof(u32); > > + } > > + > > + if (unlikely(wait && max)) > > + dev_warn(priv->dev, "timeout might be not properly set\n"); > > Is this really necessary? Better to choose proper timeout than > providing this warning message. In rare cases if the timeout could > occur due to some reason (may be a hardware fault) print appropriate > warning message. It is good, I will choose the proper timeout and remove the log in the next one. > > > + return retval || !wait ? retval : -EIO; > > +} > > Set retavl to mtk_rng_wait_ready and return retval. > Maybe i didn't get your points exactly. Adding some explanation about thoughts here. "return retval || !wait ? retval : -EIO;" I use can also help handling the both cases in one line which i think is elegant enough. And retval is accumulated with each round if some data's existing in hardware, so we don't return the value from mtk_rng_wait_ready(). > Regards, > Prasanna thanks for all your reviewing and suggestion Sean