Re: [PATCH 1/1] dmaengine: remove obsolete comment reference to dma_data_direction

[Vinod Koul <vinod.koul@intel.com>]

On Sat, Dec 14, 2013 at 02:20:27PM +0100, Gerhard Sittig wrote:
> On Wed, Dec 11, 2013 at 10:07 +0400, Alexander Popov wrote:
> > 
> > enum dma_transfer_direction is currently used
> > in struct dma_slave_config, so update the comment
> > 
> > Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> > ---
> >  include/linux/dmaengine.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 41cf0c3..bd6b882 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -305,9 +305,8 @@ enum dma_slave_buswidth {
> >  /**
> >   * struct dma_slave_config - dma slave channel runtime config
> >   * @direction: whether the data shall go in or out on this slave
> > - * channel, right now. DMA_TO_DEVICE and DMA_FROM_DEVICE are
> > - * legal values, DMA_BIDIRECTIONAL is not acceptable since we
> > - * need to differentiate source and target addresses.
> > + * channel, right now. DMA_MEM_TO_DEV and DMA_DEV_TO_MEM are
> > + * legal values.
> >   * @src_addr: this is the physical address where DMA slave data
> >   * should be read (RX), if the source is memory this argument is
> >   * ignored.
> 
> While I agree with the change to the comment text, I'd put the
> description of the patch differently.
> 
> You are not removing an obsolete reference, but you are fixing an
> erroneous comment.  It's not that dma_data_direction would have
> become obsolete, instead it's that dma_slave_config was
> documented wrongly.  The previous comment used the wrong data
> type for one of its members and thus discussed inappropriate
> values out of the type's domain.  Maybe "fix incorrect kerneldoc
> for struct dma_slave_config" or something similar could be a
> better title.  Especially for those who read the log later and
> neither have the patch nor its description at hand.
comment was apt when it was created, it wanst updated at the time of conversion!

--
~Vinod
> 
> And you might improve the description's body.  Something like
> "the 'direction' member of 'struct dma_slave_config' is of data
> type 'enum dma_transfer_direction', so update the 'struct
> dma_slave_config' kerneldoc to refer to appropriate values" or
> similar.
> 
> 
> virtually yours
> Gerhard Sittig
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--