Hello, On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote: > Hi Theo, > > ... > > > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > > +{ > > + if (priv->timeout_usecs < jiffies_to_usecs(1)) { > > + unsigned long timeout_usecs = priv->timeout_usecs; > > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > > + > > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); > > + } else { > > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); > > + > > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); > > + } > > + > > + return priv->xfer_done; > > You could eventually write this as > > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > { > if (priv->timeout_usecs < jiffies_to_usecs(1)) { > ... > > return !wait_event_hrtimeout(...); > } > > ... > return wait_event_timeout(...); > } > > It looks a bit cleaner to me... your choice. The full block would become: static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) { if (priv->timeout_usecs < jiffies_to_usecs(1)) { unsigned long timeout_usecs = priv->timeout_usecs; ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); } return wait_event_timeout(priv->xfer_wq, priv->xfer_done, usecs_to_jiffies(priv->timeout_usecs)); } Three things: - Deindenting the jiffy timeout case means no variable declaration after the if-block. This is fine from my point-of-view. - It means we depend on the half-mess that are return values from wait_event_*timeout() macros. I wanted to avoid that because it looks like an error when you read the above code and see one is negated while the other is not. - Also, I'm not confident in casting either return value to bool; what happens if either macro returns an error? This is a theoretical case that shouldn't happen, but behavior might change at some point or bugs could occur. We know priv->xfer_done will give us the right answer. My preference still goes to the original version, but I'm happy we are having a discussion about this code block. > Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx> Thanks for your review Andi! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com