On Fri, Sep 25, 2020 at 3:26 PM Kent Gibson <warthog618@xxxxxxxxx> wrote: > On Fri, Sep 25, 2020 at 12:35:49PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 24, 2020 at 6:07 AM Kent Gibson <warthog618@xxxxxxxxx> wrote: > > > On Wed, Sep 23, 2020 at 06:47:28PM +0300, Andy Shevchenko wrote: > > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@xxxxxxxxx> wrote: ... > > > > > + if (!line->timestamp_ns) { > > > > > + le.timestamp_ns = ktime_get_ns(); > > > > > + if (lr->num_lines != 1) > > > > > + line->req_seqno = atomic_inc_return(&lr->seqno); > > > > > + } else { > > > > > + le.timestamp_ns = line->timestamp_ns; > > > > > > + } > > > > > > > > Ditto. > > > > > > Firstly, drawn from lineevent_irq_thread() which is structured this way. > > > > > > In this case the comment relates to the condition being true, so > > > re-ordering the if/else would be confusing - unless the comment were > > > moved into the corresponding body?? > > > > Yes. > > > > Does that mean I should re-order and move the comment into the body? > That would work for me - the normal case is line->timestamp_ns being > set. Yes, that's what I meant. ... > > > From gpiod_to_irq(): > > > > > > /* Zero means NO_IRQ */ > > > if (!retirq) > > > return -ENXIO; > > > > > > so it can't even return a 0 :-| - we're just being cautious. > > > > I would drop = part then. > > > > ok, but you'd better not come after me in a subsequent review for not > checking the 0 case! For IRQ?! Maybe if I'll be drunk (quite unlikely). I really don't like to check IRQ against 0. To me it should be transparent to the caller. If IRQ == 0 in certain API or entirely in Linux is considered NO_IRQ, then it should be either correctly handled (means all following actions on it shouldn't fail, or it shouldn't be returned in the first place). -- With Best Regards, Andy Shevchenko