On Wed, Jun 11, 2025 at 04:15:04PM +0100, Jonathan Cameron wrote: > On Wed, 11 Jun 2025 16:49:34 +0200 > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote: > > On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko > > <andy.shevchenko@xxxxxxxxx> wrote: > > > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote: ... > > > > - return adxl313_fifo_push(indio_dev, samples); > > > > + ret = adxl313_fifo_push(indio_dev, samples); > > > > > > This is not needed... > > > > > > > IMHO this will be needed, or shall be needed in the follow up context. > > > > The [going to be renamed] function push_events() shall evaluate the > > interrupt status register for the events the driver can handle and > > also eventually drain the FIFO in case of watermark. It shall > > distinguish between failure, events / drain the FIFO which can be > > handled, and events which cannot be handled so far. It's not a if / > > else, there can be some event, and some fifo data. Therefore I'd like > > not a simple return here, but init a ret var. > > > > I interpreted your reviews, to change the particular implementation as > > if there was just activity. Then in a follow up patch, rewrite it > > again, now to distinguish just bewteen just activity and inactivity > > e.g. by if/else. Eventually rewrite it by a third approach to > > distinghish activity, inactivity, AC-coupled activity and AC-coupled > > inactivity, might be now switch/case. Eventually you might complain > > that my patches contain way too much modification of every line in > > every patch. > > > > I'd rather like to start right away with the final structure with just > > the first element - e.g. "activity" - leads to results like the above. > > Less churn among patches, but having just one element looks like > > having taken an over-complicated approach. > > I'd do the from the first but with the comment up with where ret is > declared. > > > Perhaps it's my patch split? Unsure, I tried to note in the commit message: > > > This is a preparatory patch. Some of the definitions and functions are > > > supposed to be extended for inactivity later on. > > Perhaps it needs more feedback here? > > > > Another example is seting up the read/write_event_config() or > > read/write_event_value() functions. I mean, eventually this will > > become a switch/case implementation. Of course with just one element > > switch/case seems to be obvious overkill. Going by your advice, I > > changed it to if(!..) return, it's definitely cleaner. Definitely in > > the follow up patches this will be rewritten, though. > Don't do that. Just use the switch from the start. But at the same time if switch becomes nested and 2+ levels, it's better to split the inner parts to the helpr functions or so. Doing a switch with 2+ levels looks ugly independently on the approach taken. > Sometimes we will give review feedback that doesn't take the whole > series into account (because it takes much longer to review a full series > then reread the feedback to spot anything that turned out to be due > to a later change) In those cases it is fine to just reply to the > comment with - "The switch gathers additional elements in patches X,Y,Z > and so is introduced in this first patch to reduce churn. Indeed. > > Please, let me know what is the best approach or what I can improve to > > avoid such "ping pong patching" as you name it? > > > > Might be that you're right here in this particular case, but then it > > would be better to discuss the final structure, isn't it? -- With Best Regards, Andy Shevchenko
