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

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

[Andy Shevchenko]

The 2afccbd283ae ("spi: dw: Discard static DW DMA slave structures")
did a clean up of global variables, which is fine, but messed up with
the carefully provided information in the custom DMA slave structures.
There reader can find an assignment of the DMA request lines in use.

Partially revert the above mentioned commit to restore readability
and maintainability of the code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: rebased against latest spi/for-next
 drivers/spi/spi-dw-dma.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index 1b96cec6d8cd..53d5257662e8 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -61,10 +61,8 @@ static void dw_spi_dma_maxburst_init(struct dw_spi *dws)
 
 static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 {
-	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;
 	struct pci_dev *dma_dev;
 	dma_cap_mask_t mask;
 
@@ -80,14 +78,14 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 	dma_cap_set(DMA_SLAVE, mask);
 
 	/* 1. Init rx channel */
-	slave.dma_dev = &dma_dev->dev;
-	dws->rxchan = dma_request_channel(mask, dw_spi_dma_chan_filter, &slave);
+	rx->dma_dev = &dma_dev->dev;
+	dws->rxchan = dma_request_channel(mask, dw_spi_dma_chan_filter, rx);
 	if (!dws->rxchan)
 		goto err_exit;
 
 	/* 2. Init tx channel */
-	slave.dst_id = 1;
-	dws->txchan = dma_request_channel(mask, dw_spi_dma_chan_filter, &slave);
+	tx->dma_dev = &dma_dev->dev;
+	dws->txchan = dma_request_channel(mask, dw_spi_dma_chan_filter, tx);
 	if (!dws->txchan)
 		goto free_rxchan;
 
-- 
2.26.2

[PATCH v2 2/2] spi: dw: Refactor mid_spi_dma_setup() to separate DMA and IRQ config

[Andy Shevchenko]

It's better to understand what bits are set for DMA and for IRQ handling
in mid_spi_dma_setup() if they are grouped accordingly. Thus,
refactor mid_spi_dma_setup() to separate DMA and IRQ configuration.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: rebased against latest spi/for-next

 drivers/spi/spi-dw-dma.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
index 53d5257662e8..5986c520b196 100644
--- a/drivers/spi/spi-dw-dma.c
+++ b/drivers/spi/spi-dw-dma.c
@@ -375,17 +375,17 @@ static int dw_spi_dma_setup(struct dw_spi *dws, struct spi_transfer *xfer)
 	dw_writel(dws, DW_SPI_DMARDLR, dws->rxburst - 1);
 	dw_writel(dws, DW_SPI_DMATDLR, dws->fifo_len - dws->txburst);
 
-	if (xfer->tx_buf) {
+	if (xfer->tx_buf)
 		dma_ctrl |= SPI_DMA_TDMAE;
-		imr |= SPI_INT_TXOI;
-	}
-	if (xfer->rx_buf) {
+	if (xfer->rx_buf)
 		dma_ctrl |= SPI_DMA_RDMAE;
-		imr |= SPI_INT_RXUI | SPI_INT_RXOI;
-	}
 	dw_writel(dws, DW_SPI_DMACR, dma_ctrl);
 
 	/* Set the interrupt mask */
+	if (xfer->tx_buf)
+		imr |= SPI_INT_TXOI;
+	if (xfer->rx_buf)
+		imr |= SPI_INT_RXUI | SPI_INT_RXOI;
 	spi_umask_intr(dws, imr);
 
 	reinit_completion(&dws->dma_completion);
-- 
2.26.2

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

[Serge Semin]

On Fri, May 29, 2020 at 09:31:49PM +0300, Andy Shevchenko wrote:
> The 2afccbd283ae ("spi: dw: Discard static DW DMA slave structures")
> did a clean up of global variables, which is fine, but messed up with
> the carefully provided information in the custom DMA slave structures.
> There reader can find an assignment of the DMA request lines in use.
> 
> Partially revert the above mentioned commit to restore readability
> and maintainability of the code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: rebased against latest spi/for-next
>  drivers/spi/spi-dw-dma.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> index 1b96cec6d8cd..53d5257662e8 100644
> --- a/drivers/spi/spi-dw-dma.c
> +++ b/drivers/spi/spi-dw-dma.c
> @@ -61,10 +61,8 @@ static void dw_spi_dma_maxburst_init(struct dw_spi *dws)
>  
>  static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  {
> -	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?

-Sergey

>  	struct pci_dev *dma_dev;
>  	dma_cap_mask_t mask;
>  
> @@ -80,14 +78,14 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  	dma_cap_set(DMA_SLAVE, mask);
>  
>  	/* 1. Init rx channel */
> -	slave.dma_dev = &dma_dev->dev;
> -	dws->rxchan = dma_request_channel(mask, dw_spi_dma_chan_filter, &slave);
> +	rx->dma_dev = &dma_dev->dev;
> +	dws->rxchan = dma_request_channel(mask, dw_spi_dma_chan_filter, rx);
>  	if (!dws->rxchan)
>  		goto err_exit;
>  
>  	/* 2. Init tx channel */
> -	slave.dst_id = 1;
> -	dws->txchan = dma_request_channel(mask, dw_spi_dma_chan_filter, &slave);
> +	tx->dma_dev = &dma_dev->dev;
> +	dws->txchan = dma_request_channel(mask, dw_spi_dma_chan_filter, tx);
>  	if (!dws->txchan)
>  		goto free_rxchan;
>  
> -- 
> 2.26.2
> 

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

[Andy Shevchenko]

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:
> > The 2afccbd283ae ("spi: dw: Discard static DW DMA slave structures")
> > did a clean up of global variables, which is fine, but messed up with
> > the carefully provided information in the custom DMA slave structures.
> > There reader can find an assignment of the DMA request lines in use.
> > 
> > Partially revert the above mentioned commit to restore readability
> > and maintainability of the code.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: rebased against latest spi/for-next
> >  drivers/spi/spi-dw-dma.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > index 1b96cec6d8cd..53d5257662e8 100644
> > --- a/drivers/spi/spi-dw-dma.c
> > +++ b/drivers/spi/spi-dw-dma.c
> > @@ -61,10 +61,8 @@ static void dw_spi_dma_maxburst_init(struct dw_spi *dws)
> >  
> >  static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> >  {
> > -	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.

-- 
With Best Regards,
Andy Shevchenko

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

[Serge Semin]

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:
> > > The 2afccbd283ae ("spi: dw: Discard static DW DMA slave structures")
> > > did a clean up of global variables, which is fine, but messed up with
> > > the carefully provided information in the custom DMA slave structures.
> > > There reader can find an assignment of the DMA request lines in use.
> > > 
> > > Partially revert the above mentioned commit to restore readability
> > > and maintainability of the code.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > > v2: rebased against latest spi/for-next
> > >  drivers/spi/spi-dw-dma.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> > > index 1b96cec6d8cd..53d5257662e8 100644
> > > --- a/drivers/spi/spi-dw-dma.c
> > > +++ b/drivers/spi/spi-dw-dma.c
> > > @@ -61,10 +61,8 @@ static void dw_spi_dma_maxburst_init(struct dw_spi *dws)
> > >  
> > >  static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
> > >  {
> > > -	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.

So no response to a review comment? Shall I do the same when get a review from
you?.)

I am not asking about the whole patch purpose. You know what I think about it.
My question was about why *tx and *rx pointers are required? Just wondering, I
may misunderstand something... As I see it you could use dma_tx and dma_rx here
directly with the same level of readability.

-Sergey
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Andy Shevchenko]

On Fri, May 29, 2020 at 10:05 PM Serge Semin <fancer.lancer@gmail.com> 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.

> I am not asking about the whole patch purpose. You know what I think about it.
> My question was about why *tx and *rx pointers are required? Just wondering, I
> may misunderstand something... As I see it you could use dma_tx and dma_rx here
> directly with the same level of readability.

I'll consider this in case v3 will be needed, thanks.

-- 
With Best Regards,
Andy Shevchenko

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

[Mark Brown]

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

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.

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

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

[Mark Brown]

On Fri, 29 May 2020 21:31:49 +0300, Andy Shevchenko wrote:
> The 2afccbd283ae ("spi: dw: Discard static DW DMA slave structures")
> did a clean up of global variables, which is fine, but messed up with
> the carefully provided information in the custom DMA slave structures.
> There reader can find an assignment of the DMA request lines in use.
> 
> Partially revert the above mentioned commit to restore readability
> and maintainability of the code.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: dw: Make DMA request line assignments explicit for Intel Medfield
      commit: b3f82dc26c0d4e1348ef81e0189cb8838b8b0d22
[2/2] spi: dw: Refactor mid_spi_dma_setup() to separate DMA and IRQ config
      commit: 3d7db0f11c7ad19979a1a01cac1d379ff040e886

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Serge Semin]

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@gmail.com> 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