On 14 April 2017 at 09:28, Sean Wang <sean.wang@xxxxxxxxxxxx> wrote: > > Hi PrasannaKumar, > > Add my comments inline > >> >> 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. Missed this point. Makes sense. My previous comment about return value in mtk_rng_read is invalid as I based it on a wrong assumption. > >> > +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(). retval can be 0 only when mkt_rng_wait_ready fails, returning 0 when wait is true is confusing. Expected return value when 0 bytes is read from device and wait is true is not clearly documented. "return retval || !wait ? retval : -EIO;" is also fine. Overall the code looks good to me. You can add: Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@xxxxxxxxx>. Regards, PrasannaKumar