Re: [PATCH v2 1/2] spi: dw: Make DMA request line assignments explicit for Intel Medfield

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Fri, May 29, 2020 at 08:52:43PM +0100, Mark Brown wrote:
> On Fri, May 29, 2020 at 09:49:55PM +0300, Andy Shevchenko wrote:
> > On Fri, May 29, 2020 at 09:40:50PM +0300, Serge Semin wrote:
> 
> > > > -	struct dw_dma_slave slave = {
> > > > -		.src_id = 0,
> > > > -		.dst_id = 0
> > > > -	};
> 
> > > > +	struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx;
> > > > +	struct dw_dma_slave dma_rx = { .src_id = 0 }, *rx = &dma_rx;
> 
> > > You know my attitude to these changes.) But anyway what's the point in having
> > > the *tx and *rx pointers here? Without any harm to the readability you can use
> > > the structures names directly, don't you?
> 
> > I will wait for Mark to decide.
> 

> Like I said before I don't particularly care either way, I've queued the
> patch to apply but really I'd rather that the people working on the
> driver could come to some sort of agreement here.

Mark, as you can see that's not always possible that easy to come to an
agreement. Sometimes like this time either both solutions are very similar,
or both have some pros and cons, so a better one just can't be chosen. In that
case to save a time I prefer to stop arguing and just ask a maintainer of which
one would be better for him. In any case I don't deny to respond to a review
comment and I normally justify why I think my solution is also acceptable or
why I don't see the proposed modification is better, whoever that comment
provided.

On Fri, May 29, 2020 at 10:21:35PM +0300, Andy Shevchenko wrote:
> On Fri, May 29, 2020 at 10:05 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> > On Fri, May 29, 2020 at 09:49:55PM +0300, Andy Shevchenko wrote:
> > > On Fri, May 29, 2020 at 09:40:50PM +0300, Serge Semin wrote:
> > > > On Fri, May 29, 2020 at 09:31:49PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > You know my attitude to these changes.) But anyway what's the point in having
> > > > the *tx and *rx pointers here? Without any harm to the readability you can use
> > > > the structures names directly, don't you?
> > >
> > > I will wait for Mark to decide.
> >
> > So no response to a review comment? Shall I do the same when get a review from
> > you?.)
> 
> This patch is result of you insisting on your version of things when I
> tried to explain you that it's not how it should be done. You pushed
> your vision. Mark proposed to submit your changes and consider mine
> which I agreed on. I will wait for him.
> 

Andy, that's not exactly the way the discussion evolved. You proposed a solution,
you thought would be better, justified it as being more readable and maintainable.
I considered your proposition and decided that pros didn't worth the cons,
responded to you why I'd better stick with my patch. Since you insisted on
implementing your solution I asked a higher level arbiter to make a final
decision in order to stop bikeshedding around that part of the patch. Please note,
I didn't refuse to respond, I didn't reject your review, I didn't ignore your
comment. I considered what you said, we had a discussion proposing
justifications for our solutions and only then I asked Mark to give us his
last word. That was my right to do. See the difference between your response
and mine.

In anyway regarding how it "should or shouldn't be done". The patchsets you had
a chance to see and review weren't my first ones. I've sent patches to different
kernel subsystems before and not once and nearly every maintainer/reviewer had a
vision of what should or shouldn't be done sometimes contradicting to each other,
what is right or wrong including the way the patches are formatted,
merged/squashed or split up, moved, inter-mixed and so on. I didn't ignore any
of such comments, and took them into account if they had been justified enough.
But when it came to a situation like this, then "should or shouldn't" aren't
right verbs to say. It's more like personal believe, personal preference rather
than a selection of a right thing to do. The same is applicable to your comment
regarding squashing the patches in the DMA-related series and to the patches
like that. Don't get me wrong I very appreciate reviews you've done and most
likely will do for my patches (since there are going to be two more series
submitted for the DW APB SSI driver). A lot of your comments were very helpful
and indeed implementing some of what you suggested made the patchsets better.
But please don't assume that that valuable comments made everything you think is
the only right thing to do, give a submitter a chance to be right too and
let a bikeshadding go when it really doesn't worth to go on an endless arguing
especially with respect to the situations like we've got here.

-Sergey