diff mbox series

[v3,03/16] spi: dw: Discard static DW DMA slave structures

Message ID 20200521012206.14472-4-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State Superseded
Commit 2afccbd283ae63072391ea17ac1d772ed9d33749
Headers show
Series spi: dw: Add generic DW DMA controller support | expand

Commit Message

Serge Semin May 21, 2020, 1:21 a.m. UTC 
Having them declared is redundant since each struct dw_dma_chan has
the same structure embedded and the structure from the passed dma_chan
private pointer will be copied there as a result of the next calls
chain:
dma_request_channel() -> find_candidate() -> dma_chan_get() ->
device_alloc_chan_resources() = dwc_alloc_chan_resources() ->
dw_dma_filter().
So just remove the static dw_dma_chan structures and use a locally
declared data instance with dst_id/src_id set to the same values as
the static copies used to have.

Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org

---

Changelog v3:
- Explicitly initialize the dw_dma_slave members on stack.
---
 drivers/spi/spi-dw-mid.c | 19 ++++++++-----------
 drivers/spi/spi-dw.h     |  2 --
 2 files changed, 8 insertions(+), 13 deletions(-)

Comments

Andy Shevchenko May 21, 2020, 9:57 a.m. UTC | #1 
On Thu, May 21, 2020 at 4:23 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> Having them declared is redundant since each struct dw_dma_chan has
> the same structure embedded and the structure from the passed dma_chan
> private pointer will be copied there as a result of the next calls
> chain:
> dma_request_channel() -> find_candidate() -> dma_chan_get() ->
> device_alloc_chan_resources() = dwc_alloc_chan_resources() ->
> dw_dma_filter().
> So just remove the static dw_dma_chan structures and use a locally
> declared data instance with dst_id/src_id set to the same values as
> the static copies used to have.

...

> - Explicitly initialize the dw_dma_slave members on stack.

Thanks for an update, but that's not what I asked for...

> -static struct dw_dma_slave mid_dma_tx = { .dst_id = 1 };
> -static struct dw_dma_slave mid_dma_rx = { .src_id = 0 };

>  static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  {
> +       struct dw_dma_slave slave = {
> +               .src_id = 0,
> +               .dst_id = 0
> +       };

(It's member, and not memberS)

> -       struct dw_dma_slave *tx = dws->dma_tx;
> -       struct dw_dma_slave *rx = dws->dma_rx;

May we simple do

struct dw_dma_slave tx = { .dst_id = 1 };
struct dw_dma_slave rx = { .src_id = 0 };

please?
Andy Shevchenko May 21, 2020, 12:49 p.m. UTC | #2 
On Thu, May 21, 2020 at 3:12 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Thu, May 21, 2020 at 12:57:17PM +0300, Andy Shevchenko wrote:
> > On Thu, May 21, 2020 at 4:23 AM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:

...

> > Thanks for an update, but that's not what I asked for...
> >
> > > -static struct dw_dma_slave mid_dma_tx = { .dst_id = 1 };
> > > -static struct dw_dma_slave mid_dma_rx = { .src_id = 0 };
> >
> > >  static int mid_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 *tx = dws->dma_tx;
> > > -       struct dw_dma_slave *rx = dws->dma_rx;
> >
> > May we simple do
> >
> > struct dw_dma_slave tx = { .dst_id = 1 };
> > struct dw_dma_slave rx = { .src_id = 0 };
> >
> > please?
>
> Well, for me both solutions are equal

I don't think so.

> except mine consumes less stack memory.

And brought confusion and less readability. :-(

> The only reason why your solution might be better is that if DW DMA driver or
> the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> the dma_request_channel() method, which non of them do. So I'll leave this for
> Mark to decide. Mark, could you give us your final word about this?

I explained already why I prefer to see them in that form. Reader can
easily understand what request line is used for what channel.
In your code it's hidden somewhere and on top of that that _one_
structure on the stack adds more confusion.
Mark Brown May 21, 2020, 3:51 p.m. UTC | #3 
On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:

> Well, for me both solutions are equal except mine consumes less stack memory.
> The only reason why your solution might be better is that if DW DMA driver or
> the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> the dma_request_channel() method, which non of them do. So I'll leave this for
> Mark to decide. Mark, could you give us your final word about this?

Honestly I'm struggling to care either way.  I guess saving a bit of
stack is potentially useful.
Andy Shevchenko May 21, 2020, 3:56 p.m. UTC | #4 
On Thu, May 21, 2020 at 6:51 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:
>
> > Well, for me both solutions are equal except mine consumes less stack memory.
> > The only reason why your solution might be better is that if DW DMA driver or
> > the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> > the dma_request_channel() method, which non of them do. So I'll leave this for
> > Mark to decide. Mark, could you give us your final word about this?
>
> Honestly I'm struggling to care either way.  I guess saving a bit of
> stack is potentially useful.

Yes, but OTOH dropping maintainability by this is worse in my opinion.
Serge Semin May 21, 2020, 3:58 p.m. UTC | #5 
On Thu, May 21, 2020 at 04:51:43PM +0100, Mark Brown wrote:
> On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:
> 
> > Well, for me both solutions are equal except mine consumes less stack memory.
> > The only reason why your solution might be better is that if DW DMA driver or
> > the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> > the dma_request_channel() method, which non of them do. So I'll leave this for
> > Mark to decide. Mark, could you give us your final word about this?
> 
> Honestly I'm struggling to care either way.  I guess saving a bit of
> stack is potentially useful.

Settled then. Let's leave the patch as is. I suppose we've finally finished a
review except a question Feng asked to the patch:
[PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA

If you are ok with my responses, then the patchset is ready for you further
actions.

-Sergey
Andy Shevchenko May 21, 2020, 4:02 p.m. UTC | #6 
On Thu, May 21, 2020 at 6:58 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Thu, May 21, 2020 at 04:51:43PM +0100, Mark Brown wrote:
> > On Thu, May 21, 2020 at 03:12:28PM +0300, Serge Semin wrote:
> >
> > > Well, for me both solutions are equal except mine consumes less stack memory.
> > > The only reason why your solution might be better is that if DW DMA driver or
> > > the DMA engine subsystem changed the dw_dma_slave structure instance passed to
> > > the dma_request_channel() method, which non of them do. So I'll leave this for
> > > Mark to decide. Mark, could you give us your final word about this?
> >
> > Honestly I'm struggling to care either way.  I guess saving a bit of
> > stack is potentially useful.
>
> Settled then.

With whom?

> Let's leave the patch as is.

Mark, should I send a partial revert afterwards in this case?
I'm not fully satisfied with it.
Mark Brown May 21, 2020, 4:39 p.m. UTC | #7 
On Thu, May 21, 2020 at 07:02:32PM +0300, Andy Shevchenko wrote:
> On Thu, May 21, 2020 at 6:58 PM Serge Semin

> > Let's leave the patch as is.

> Mark, should I send a partial revert afterwards in this case?
> I'm not fully satisfied with it.

That might be a suitable way to keep the peace here.  You are clearly
both much more passionate about this choice than I am.
Andy Shevchenko May 21, 2020, 4:44 p.m. UTC | #8 
On Thu, May 21, 2020 at 7:39 PM Mark Brown <broonie@kernel.org> wrote:
> On Thu, May 21, 2020 at 07:02:32PM +0300, Andy Shevchenko wrote:
> > On Thu, May 21, 2020 at 6:58 PM Serge Semin
>
> > > Let's leave the patch as is.
>
> > Mark, should I send a partial revert afterwards in this case?
> > I'm not fully satisfied with it.
>
> That might be a suitable way to keep the peace here.  You are clearly
> both much more passionate about this choice than I am.

Okay, will work for me, thanks!
Serge Semin May 21, 2020, 5:29 p.m. UTC | #9 
On Thu, May 21, 2020 at 05:39:04PM +0100, Mark Brown wrote:
> On Thu, May 21, 2020 at 07:02:32PM +0300, Andy Shevchenko wrote:
> > On Thu, May 21, 2020 at 6:58 PM Serge Semin
> 
> > > Let's leave the patch as is.
> 
> > Mark, should I send a partial revert afterwards in this case?
> > I'm not fully satisfied with it.
> 
> That might be a suitable way to keep the peace here.  You are clearly
> both much more passionate about this choice than I am.

Fine with me.)

-Sergey
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
index cf99832ba271..8446bad0528c 100644
--- a/drivers/spi/spi-dw-mid.c
+++ b/drivers/spi/spi-dw-mid.c
@@ -21,9 +21,6 @@ 
 #define RX_BUSY		0
 #define TX_BUSY		1
 
-static struct dw_dma_slave mid_dma_tx = { .dst_id = 1 };
-static struct dw_dma_slave mid_dma_rx = { .src_id = 0 };
-
 static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
 {
 	struct dw_dma_slave *s = param;
@@ -37,9 +34,11 @@  static bool mid_spi_dma_chan_filter(struct dma_chan *chan, void *param)
 
 static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 {
+	struct dw_dma_slave slave = {
+		.src_id = 0,
+		.dst_id = 0
+	};
 	struct pci_dev *dma_dev;
-	struct dw_dma_slave *tx = dws->dma_tx;
-	struct dw_dma_slave *rx = dws->dma_rx;
 	dma_cap_mask_t mask;
 
 	/*
@@ -54,14 +53,14 @@  static int mid_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
 	dma_cap_set(DMA_SLAVE, mask);
 
 	/* 1. Init rx channel */
-	rx->dma_dev = &dma_dev->dev;
-	dws->rxchan = dma_request_channel(mask, mid_spi_dma_chan_filter, rx);
+	slave.dma_dev = &dma_dev->dev;
+	dws->rxchan = dma_request_channel(mask, mid_spi_dma_chan_filter, &slave);
 	if (!dws->rxchan)
 		goto err_exit;
 
 	/* 2. Init tx channel */
-	tx->dma_dev = &dma_dev->dev;
-	dws->txchan = dma_request_channel(mask, mid_spi_dma_chan_filter, tx);
+	slave.dst_id = 1;
+	dws->txchan = dma_request_channel(mask, mid_spi_dma_chan_filter, &slave);
 	if (!dws->txchan)
 		goto free_rxchan;
 
@@ -386,8 +385,6 @@  static const struct dw_spi_dma_ops mfld_dma_ops = {
 
 static void dw_spi_mid_setup_dma_mfld(struct dw_spi *dws)
 {
-	dws->dma_tx = &mid_dma_tx;
-	dws->dma_rx = &mid_dma_rx;
 	dws->dma_ops = &mfld_dma_ops;
 }
 
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 81364f501b7e..60e9e430ce7b 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -146,8 +146,6 @@  struct dw_spi {
 	unsigned long		dma_chan_busy;
 	dma_addr_t		dma_addr; /* phy address of the Data register */
 	const struct dw_spi_dma_ops *dma_ops;
-	void			*dma_tx;
-	void			*dma_rx;
 
 	/* Bus interface info */
 	void			*priv;