| Message ID | 20200529183150.44149-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b3f82dc26c0d4e1348ef81e0189cb8838b8b0d22 |
| Headers | show |
| Series | [v2,1/2] spi: dw: Make DMA request line assignments explicit for Intel Medfield | expand |
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 >
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.
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 > >
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.
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.
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
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
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;
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(-)