Hi Theo, On Mon, Mar 04, 2024 at 03:32:38PM +0100, Théo Lebrun wrote: > On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote: > > > +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. sure... it's not a binding comment. Andi