Re: [PATCH 1/1] tty: serial: use dmaengine_desc_set_callback()

[Thierry Reding <thierry.reding@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4762 bytes --]

On Thu, Nov 05, 2026 at 06:16:26PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 05, 2026 at 04:34:15PM +0100, Thierry Reding wrote:
> > On Thu, Nov 05, 2026 at 03:35:20PM +0100, Krzysztof Kozlowski wrote:
> > > On 05/11/2026 15:17, Andy Shevchenko wrote:
> > > >>> ---
> > > >>>  drivers/tty/serial/8250/8250_dma.c  | 6 ++----
> > > >>>  drivers/tty/serial/8250/8250_mtk.c  | 3 +--
> > > >>>  drivers/tty/serial/8250/8250_omap.c | 6 ++----
> > > >>>  drivers/tty/serial/amba-pl011.c     | 6 ++----
> > > >>>  drivers/tty/serial/atmel_serial.c   | 6 ++----
> > > >>>  drivers/tty/serial/fsl_lpuart.c     | 6 ++----
> > > >>>  drivers/tty/serial/imx.c            | 6 ++----
> > > >>>  drivers/tty/serial/msm_serial.c     | 6 ++----
> > > >>>  drivers/tty/serial/mxs-auart.c      | 6 ++----
> > > >>>  drivers/tty/serial/pch_uart.c       | 6 ++----
> > > >>>  drivers/tty/serial/samsung_tty.c    | 6 ++----
> > > >>>  drivers/tty/serial/serial-tegra.c   | 6 ++----
> > > >>>  drivers/tty/serial/sh-sci.c         | 9 +++------
> > > >>>  drivers/tty/serial/sprd_serial.c    | 3 +--
> > > >>>  drivers/tty/serial/stm32-usart.c    | 6 ++----
> > > >>>  15 files changed, 29 insertions(+), 58 deletions(-)
> > > >>
> > > >> Reviewed-by: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Krzysztof, now look at this case. I would argue that this is even less fit than
> > > > # subpart(s) case, because it might imply anything. Again the problem is the
> > > > bundle, and for such a case (when tons of independent changes are bundled in
> > > > the same stuff, *every* Rb tag has to be explained on what has been reviewed!
> > > 
> > > I think this case is obvious - Thierry reviewed this patch and gives Rb
> > > tag, according to reviewer's statement of oversight. I don't see a
> > > problem here or what is questionable.
> > 
> > Correct. I looked at this whole thing and approve of it.
> 
> for this type of bundles it's not obvious.

I think it's obvious if you interpret the tag the way it's described in
the process documentation. If everyone has their own interpretation of
what it means we have a whole other set of problems.

> > If I review only parts of something at most I'll give an Acked-by.
> 
> Which is strange, don't you find it? The review is not an acknowledgement.
> These are two different operations.

I think review is a stronger form of acknowledgement.

> > I don't think I've ever done partial reviews. I don't think you can reasonably do
> > that
> > because if you don't know what other parts of a patch are doing, how can
> > you say that a specific hunk does what it's supposed to.
> > 
> > Patches are an atomic set of changes,
> 
> Of course, not always the case. This patch is a bundle of atomic changes
> in a single drop.

That seems like a slippery slope. It means we now need to interpret what
atomic means. If a commit is not atomic, that to me is grounds for
rejection. Maybe atomic is the wrong term here. The patch to me is still
one logical change because it applies the same transformation to all
these different drivers, and therefore it doesn't require to be split up
explicitly.

> > so I don't think it makes sense to review only parts of it. If you can't be
> > bothered to look at the entirety, that's absolutely fine of course, but in
> > that case don't provide a Reviewed-by.
> > 
> > Or, I suppose, you do and then you bear the consequences of lending
> > credibility to something that you haven't looked at or understood.
> 
> No, I review *my* part, that I understand and I confirm.

I think we need to distinguish between various levels of complexity. For
this particular case there's not much to understand, it's purely a
transformation, so you could very well review all the patch hunks
without needing more context as to the drivers.

If there were more complexity to this, then I would also argue that it'd
be better to have separate patches per driver. Most of the time
submitters tend to do that all by themselves because it's almost common
sense. But like I said, I don't think that applies here.

Also, I'd like to remind everyone that this is a community project and
if everyone is only ever going to review *their* part, it's all going to
deteriorate over time. Again, I sympathize with people not having time
to extensively review everything. However, for cases like this once you
already load enough context to review *your* part, you might as well
spend the extra minute or two reviewing the other parts, which is all
that's necessary to make this a full review.

Of course it's a bit questionable that a full review is really needed
here, in which case an Acked-by is really all you need.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]