Re: [PATCH v2 1/1] spi: use dmaengine_desc_set_callback()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Nov 04, 2026 at 06:27:40PM +0100, Krzysztof Kozlowski wrote:
> On 04/11/2026 18:23, Andy Shevchenko wrote:
> > On Wed, Nov 04, 2026 at 06:08:39PM +0100, Krzysztof Kozlowski wrote:
> >> On 04/11/2026 18:02, Krzysztof Kozlowski wrote:
> >>> On 04/11/2026 17:57, Andy Shevchenko wrote:
> >>>> On Wed, Nov 04, 2026 at 10:22:45AM -0500, Frank.Li@oss.nxp.com wrote:
> >>>>>
> >>>>> Replace open-coded assignments to callback and callback_param fields
> >>>>> of struct dma_async_tx_descriptor with calls to the
> >>>>> dmaengine_desc_set_callback() helper across SPI bus drivers.
> >>>>>
> >>>>> No functional change intended. Prepare make to struct
> >>>>> dma_async_tx_descriptor private to the DMA engine.
> >>>>
> >>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> # dw, pxa2xx, topcliff-pch
> >>>>
> >>>> (Note, you forgot to add the list of the drivers to the Rb trailers of other reviewers)
> >>>
> >>> These should be acks. RB creates false impression entire commit was
> >>> reviewed. Our docs also clearly state entire commit:
> >>>
> >>> "Reviewed-by:, instead, indicates that the patch has been reviewed and
> >>> found acceptable according to the Reviewer's Statemen"
> >>>
> >>> (not patch or part of patch)
> >>>
> >>> "By offering my Reviewed-by: tag, I state that
> >>> I have carried out a technical review of this patch to
> >>> "
> >>>
> >>> (not patch or part of patch)

Documentation doesn't cover partial reviews at all.

> >>> When you accept only part of this code, you basically Ack. I really
> >>> dislike these half-done reviews, which later appear in logs and tell you
> >>> nothing. You could have clear security bug here, but 10 review tags. How
> >>> useful tags...
> >>
> >> And let me even bring a better example. Imagine a patch, doing massive
> >> refactoring of core SPI stuff, 1000 lines of code, complex locking and
> >> also simultaneously updates drivers to new API changed in this core.
> >>
> >> Reviewing that 1000 lines of code in core SPI with complex locking takes
> >> someone 30 minutes. He gives Rb tag, gets credit.
> >>
> >> Now you come, look at SINGLE line in a driver and give Rb tag. For 10
> >> seconds job of looking at one changed call in one driver.
> >>
> >> You just Acked, you did not review "the patch" (quoting submitting patches).
> > 
> > Your example is doubtful because the patch is combined from independent pieces.
> > The Rb fits very well the listed drivers. The problem is that some of the
> > people combine changes into a bundle (and not proper per-driver series).
> 
> The example is actually real. There are many patches changing some core
> subsystem API stuff + many drivers, and they are done correct as one patch.
> 
> Looking at two lines out of 50, 100 or 1000 (here is 740!!!) is not
> reviewing "this patch". Is acking it.
> 
> All of the Rbs here are half baked, meaning they did not review "this
> patch" (quoting submitting patches document).
> 
> But sure, if we want to play such numbers game, I will be sending
> reviews for single lines.

I'm not playing anything. The problem is that the patch unites the files that
have distinct maintainers and unrelated to each other code. That's why in the
subsystems I maintain, I almost always require to split on per-driver basis.
Yes, exceptions are possible, but has to be quite rare.

-- 
With Best Regards,
Andy Shevchenko