DMAENGINE: ste_dma40: only write phy channel config first time
in [Kernel]
|
Prev: kernel:audit.c Fix warning: variable 'nlh' set but not used
Next: scripts:conf.c Fix warning: variable 'type' set but not used
From: Dan Williams on 11 Jun 2010 18:30 On Tue, Jun 1, 2010 at 5:20 AM, Linus Walleij <linus.walleij(a)stericsson.com> wrote: > We only need to write the configuration to a physical channel if > it is free, else it is already written. > > Signed-off-by: Jonas Aaberg <jonas.aberg(a)stericsson.com> > Signed-off-by: Linus Walleij <linus.walleij(a)stericsson.com> > --- > �drivers/dma/ste_dma40.c | � 19 +++++++++++++------ > �1 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c > index 4618d6c..1cd1ff7 100644 > --- a/drivers/dma/ste_dma40.c > +++ b/drivers/dma/ste_dma40.c > @@ -1211,9 +1211,9 @@ out: > �} > > �static int d40_config_chan(struct d40_chan *d40c, > - � � � � � � � � � � � � �struct stedma40_chan_cfg *info) > + � � � � � � � � � � � � �struct stedma40_chan_cfg *info, > + � � � � � � � � � � � � �bool is_free_phy) > �{ > - > � � � �/* Fill in basic CFG register values */ > � � � �d40_phy_cfg(&d40c->dma_cfg, &d40c->src_def_cfg, > � � � � � � � � � �&d40c->dst_def_cfg, d40c->log_num != D40_PHY_CHAN); > @@ -1230,8 +1230,14 @@ static int d40_config_chan(struct d40_chan *d40c, > � � � � � � � � � � � � � � � �d40c->dma_cfg.dst_dev_type * 32 + 16; > � � � �} > > - � � � /* Write channel configuration to the DMA */ > - � � � return d40_config_write(d40c); > + � � � /* > + � � � �* Only write channel configuration to the DMA if the physical > + � � � �* resource is free. In case of multiple logical channels > + � � � �* on the same physical resource, only the first write is necessary. > + � � � �*/ > + � � � if (is_free_phy) > + � � � � � � � return d40_config_write(d40c); > + � � � return 0; > �} > > �static int d40_config_memcpy(struct d40_chan *d40c) > @@ -1691,7 +1697,7 @@ static int d40_alloc_chan_resources(struct dma_chan *chan) > � � � �unsigned long flags; > � � � �struct d40_chan *d40c = > � � � � � � � �container_of(chan, struct d40_chan, chan); > - > + � � � bool is_free_phy; > � � � �spin_lock_irqsave(&d40c->lock, flags); > > � � � �d40c->completed = chan->cookie = 1; > @@ -1705,6 +1711,7 @@ static int d40_alloc_chan_resources(struct dma_chan *chan) > � � � � � � � �if (err) > � � � � � � � � � � � �goto err_alloc; > � � � �} > + � � � is_free_phy = (d40c->phy_chan == NULL); > It seems like d40_config_chan() just wants to be open coded in d40_alloc_chan_resources(). It does not have its own local variables, and the only thing that returns an error is d40_config_write(). While trying to figure out why this flag was needed I happened to notice a gratuitous use of gotos in d40_allocate_channel(). Can we make this function easier to read by just returning -EINVAL directly when those tests fail? -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dan Williams on 15 Jun 2010 01:00
On Mon, Jun 14, 2010 at 9:39 AM, Linus WALLEIJ <linus.walleij(a)stericsson.com> wrote: > Thanks Dan, > > Is it OK that we send a new version of the entire patch set trying > to address all comments? (Then hopefully you can merge forward > through the set until there is some further issue.) There other patches were fine. If you could just send replacement patches for the ones I commented on I'll get this merged and pushed out. > > On 06/12/2010 12:23 AM, Dan Williams wrote: >> On Tue, Jun 1, 2010 at 5:20 AM, Linus Walleij >> <linus.walleij(a)stericsson.com> wrote: >>> We only need to write the configuration to a physical channel if it >>> is free, else it is already written. >>> >>> Signed-off-by: Jonas Aaberg <jonas.aberg(a)stericsson.com> >>> Signed-off-by: Linus Walleij <linus.walleij(a)stericsson.com> >>> --- >>> �drivers/dma/ste_dma40.c | � 19 +++++++++++++------ >>> �1 files changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c index >>> 4618d6c..1cd1ff7 100644 >>> --- a/drivers/dma/ste_dma40.c >>> +++ b/drivers/dma/ste_dma40.c >>> @@ -1211,9 +1211,9 @@ out: >>> �} >>> >>> �static int d40_config_chan(struct d40_chan *d40c, >>> - � � � � � � � � � � � � �struct stedma40_chan_cfg *info) >>> + � � � � � � � � � � � � �struct stedma40_chan_cfg *info, >>> + � � � � � � � � � � � � �bool is_free_phy) >>> �{ >>> - >>> � � � �/* Fill in basic CFG register values */ >>> � � � �d40_phy_cfg(&d40c->dma_cfg, &d40c->src_def_cfg, >>> � � � � � � � � � �&d40c->dst_def_cfg, d40c->log_num != >>> D40_PHY_CHAN); @@ -1230,8 +1230,14 @@ static int d40_config_chan(struct d40_chan *d40c, >>> � � � � � � � � � � � � � � � �d40c->dma_cfg.dst_dev_type * 32 + 16; >>> � � � �} >>> >>> - � � � /* Write channel configuration to the DMA */ >>> - � � � return d40_config_write(d40c); >>> + � � � /* >>> + � � � �* Only write channel configuration to the DMA if the physical >>> + � � � �* resource is free. In case of multiple logical channels >>> + � � � �* on the same physical resource, only the first write is necessary. >>> + � � � �*/ >>> + � � � if (is_free_phy) >>> + � � � � � � � return d40_config_write(d40c); >>> + � � � return 0; >>> �} >>> >>> �static int d40_config_memcpy(struct d40_chan *d40c) @@ -1691,7 >>> +1697,7 @@ static int d40_alloc_chan_resources(struct dma_chan *chan) >>> � � � �unsigned long flags; >>> � � � �struct d40_chan *d40c = >>> � � � � � � � �container_of(chan, struct d40_chan, chan); >>> - >>> + � � � bool is_free_phy; >>> � � � �spin_lock_irqsave(&d40c->lock, flags); >>> >>> � � � �d40c->completed = chan->cookie = 1; @@ -1705,6 +1711,7 @@ >>> static int d40_alloc_chan_resources(struct dma_chan *chan) >>> � � � � � � � �if (err) >>> � � � � � � � � � � � �goto err_alloc; >>> � � � �} >>> + � � � is_free_phy = (d40c->phy_chan == NULL); >>> >> >> It seems like d40_config_chan() just wants to be open coded in >> d40_alloc_chan_resources(). �It does not have its own local variables, >> and the only thing that returns an error is d40_config_write(). > > True. There are a few more such smaller functions that shall be removed. > I will do the clean up in a second version of this patch set, ok? Ok, or it could be incremental to this set... either way is fine. >> While trying to figure out why this flag was needed I happened to >> notice a gratuitous use of gotos in d40_allocate_channel(). �Can we >> make this function easier to read by just returning -EINVAL directly >> when those tests fail? > > We'll try to see if I can make this function easier to read with less gotos. > > Yours, > Linus Walleij Thanks, Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |