[PATCH 1/2] dw_dmac: check direction properly in dw_dma_cyclic_prep

[PATCH 1/2] dw_dmac: check direction properly in dw_dma_cyclic_prep

[Andy Shevchenko]

dma_transfer_direction is a normal enum. It means we can't usually use the
values as bit fields. Let's adjust this check and move it above the usage of
the direction parameter.

This patch introduces helper function is_slave_direction() that will be used
later in other places of the code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index a4a5c80..771aa15 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -36,6 +36,11 @@
  * which does not support descriptor writeback.
  */
 
+static inline bool is_slave_direction(enum dma_transfer_direction direction)
+{
+	return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
+}
+
 static inline unsigned int dwc_get_dms(struct dw_dma_slave *slave)
 {
 	return slave ? slave->dst_master : 0;
@@ -1336,6 +1341,9 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 
 	retval = ERR_PTR(-EINVAL);
 
+	if (unlikely(!is_slave_direction(direction)))
+		goto out_err;
+
 	if (direction == DMA_MEM_TO_DEV)
 		reg_width = __ffs(sconfig->dst_addr_width);
 	else
@@ -1350,8 +1358,6 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 		goto out_err;
 	if (unlikely(buf_addr & ((1 << reg_width) - 1)))
 		goto out_err;
-	if (unlikely(!(direction & (DMA_MEM_TO_DEV | DMA_DEV_TO_MEM))))
-		goto out_err;
 
 	retval = ERR_PTR(-ENOMEM);
 
-- 
1.7.10.4

[PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Andy Shevchenko]

Currently for the slave transfers the driver requires a custom slave
configuration to be present. Nevertheless, in some cases we need only the
request line as an additional information to the generic slave configuration.
The request line is provided by slave_id parameter of the dma_slave_config
structure. That's why the custom slave configuration could be optional for such
cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 771aa15..baebf81 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -41,6 +41,11 @@ static inline bool is_slave_direction(enum dma_transfer_direction direction)
 	return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
 }
 
+static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
+{
+	return is_slave_direction(sconfig->direction);
+}
+
 static inline unsigned int dwc_get_dms(struct dw_dma_slave *slave)
 {
 	return slave ? slave->dst_master : 0;
@@ -55,11 +60,12 @@ static inline unsigned int dwc_get_sms(struct dw_dma_slave *slave)
 		struct dw_dma_slave *__slave = (_chan->private);	\
 		struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan);	\
 		struct dma_slave_config	*_sconfig = &_dwc->dma_sconfig;	\
+		bool _is_slave = dwc_is_slave(_sconfig);	\
 		int _dms = dwc_get_dms(__slave);		\
 		int _sms = dwc_get_sms(__slave);		\
-		u8 _smsize = __slave ? _sconfig->src_maxburst :	\
+		u8 _smsize = _is_slave ? _sconfig->src_maxburst :	\
 			DW_DMA_MSIZE_16;			\
-		u8 _dmsize = __slave ? _sconfig->dst_maxburst :	\
+		u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :	\
 			DW_DMA_MSIZE_16;			\
 								\
 		(DWC_CTLL_DST_MSIZE(_dmsize)			\
@@ -330,7 +336,7 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 	list_splice_init(&desc->tx_list, &dwc->free_list);
 	list_move(&desc->desc_node, &dwc->free_list);
 
-	if (!dwc->chan.private) {
+	if (!dwc_is_slave(&dwc->dma_sconfig)) {
 		struct device *parent = chan2parent(&dwc->chan);
 		if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
 			if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
@@ -809,9 +815,11 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 
 	dev_vdbg(chan2dev(chan), "%s\n", __func__);
 
-	if (unlikely(!dws || !sg_len))
+	if (unlikely(!is_slave_direction(direction) || !sg_len))
 		return NULL;
 
+	sconfig->direction = direction;
+
 	prev = first = NULL;
 
 	switch (direction) {
@@ -983,8 +991,8 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 {
 	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
 
-	/* Check if it is chan is configured for slave transfers */
-	if (!chan->private)
+	/* Check if chan will be configured for slave transfers */
+	if (!dwc_is_slave(sconfig))
 		return -EINVAL;
 
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
@@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 	if (unlikely(!is_slave_direction(direction)))
 		goto out_err;
 
+	sconfig->direction = direction;
+
 	if (direction == DMA_MEM_TO_DEV)
 		reg_width = __ffs(sconfig->dst_addr_width);
 	else
@@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
 		channel_clear_bit(dw, CH_EN, dwc->mask);
 
 		dwc->dw = dw;
+		dwc->dma_sconfig.direction = DMA_TRANS_NONE;
 
 		/* hardware configuration */
 		if (autocfg) {
-- 
1.7.10.4

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[viresh kumar]

On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> +static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
> +{
> +       return is_slave_direction(sconfig->direction);
> +}

I will not buy this one. Why hide the real implementation, call
is_slave_direction()
directly.

> @@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
> +       sconfig->direction = direction;
> +

> @@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
> +               dwc->dma_sconfig.direction = DMA_TRANS_NONE;

Why do you need above changes??

--
viresh

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Andy Shevchenko]

On Thu, 2012-11-15 at 20:28 +0530, viresh kumar wrote: 
> On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> 
> > +static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
> > +{
> > +       return is_slave_direction(sconfig->direction);
> > +}
> 
> I will not buy this one. Why hide the real implementation, call
> is_slave_direction()
> directly.

There is no strong reason to keep it so.

> > @@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
> > +       sconfig->direction = direction;
> > +
> 
> > @@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
> > +               dwc->dma_sconfig.direction = DMA_TRANS_NONE;
> 
> Why do you need above changes??

This one is not needed indeed. But we have to look after default (0)
enum value.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Viresh Kumar]

On 15 November 2012 20:51, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2012-11-15 at 20:28 +0530, viresh kumar wrote:
>> On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
>>
>> > +static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
>> > +{
>> > +       return is_slave_direction(sconfig->direction);
>> > +}
>>
>> I will not buy this one. Why hide the real implementation, call
>> is_slave_direction()
>> directly.
>
> There is no strong reason to keep it so.
>
>> > @@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
>> > +       sconfig->direction = direction;
>> > +
>>
>> > @@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
>> > +               dwc->dma_sconfig.direction = DMA_TRANS_NONE;
>>
>> Why do you need above changes??
>
> This one is not needed indeed. But we have to look after default (0)
> enum value.

I am asking about all lines where you are doing sconfig->direction = foo;

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Andy Shevchenko]

On Thu, 2012-11-15 at 20:54 +0530, Viresh Kumar wrote: 
> On 15 November 2012 20:51, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, 2012-11-15 at 20:28 +0530, viresh kumar wrote:
> >> On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
> >> <andriy.shevchenko@linux.intel.com> wrote:
> >> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> >>
> >> > +static inline bool dwc_is_slave(struct dma_slave_config *sconfig)
> >> > +{
> >> > +       return is_slave_direction(sconfig->direction);
> >> > +}
> >>
> >> I will not buy this one. Why hide the real implementation, call
> >> is_slave_direction()
> >> directly.
> >
> > There is no strong reason to keep it so.
> >
> >> > @@ -1344,6 +1352,8 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
> >> > +       sconfig->direction = direction;
> >> > +
> >>
> >> > @@ -1718,6 +1728,7 @@ static int __devinit dw_probe(struct platform_device *pdev)
> >> > +               dwc->dma_sconfig.direction = DMA_TRANS_NONE;
> >>
> >> Why do you need above changes??
> >
> > This one is not needed indeed. But we have to look after default (0)
> > enum value.
> 
> I am asking about all lines where you are doing sconfig->direction = foo;

Well, the prep_* should assign the value due to changes of check in the
dwc_descriptor_complete. Otherwise we will potentially skip some
important piece of code.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Viresh Kumar]

On 15 November 2012 20:57, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Well, the prep_* should assign the value due to changes of check in the
> dwc_descriptor_complete. Otherwise we will potentially skip some
> important piece of code.

What i meant to say was, set_runtime_config() must have already done this part.

--
viresh

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Andy Shevchenko]

On Thu, Nov 15, 2012 at 5:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 15 November 2012 20:57, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> Well, the prep_* should assign the value due to changes of check in the
>> dwc_descriptor_complete. Otherwise we will potentially skip some
>> important piece of code.
>
> What i meant to say was, set_runtime_config() must have already done this part.

On one hand it is true. On the other - *_prep* functions use
explicitly passed parameter. I doubt there is a consistency between
value in slave config passed via dwc_control and value passed as
explicit function parameter.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Viresh Kumar]

On 15 November 2012 23:28, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Nov 15, 2012 at 5:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 15 November 2012 20:57, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>> Well, the prep_* should assign the value due to changes of check in the
>>> dwc_descriptor_complete. Otherwise we will potentially skip some
>>> important piece of code.
>>
>> What i meant to say was, set_runtime_config() must have already done this part.
>
> On one hand it is true. On the other - *_prep* functions use
> explicitly passed parameter. I doubt there is a consistency between
> value in slave config passed via dwc_control and value passed as
> explicit function parameter.

I believe it should be consistent.

@Vinod: Why have we duplicated direction? Once in prep_* and then in
slave_config?

--
viresh

Re: [PATCH 2/2] dw_dmac: make usage of dw_dma_slave optional

[Andy Shevchenko]

On Fri, 2012-11-16 at 09:59 +0530, Viresh Kumar wrote: 
> On 15 November 2012 23:28, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Nov 15, 2012 at 5:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >> On 15 November 2012 20:57, Andy Shevchenko
> >> <andriy.shevchenko@linux.intel.com> wrote:
> >>> Well, the prep_* should assign the value due to changes of check in the
> >>> dwc_descriptor_complete. Otherwise we will potentially skip some
> >>> important piece of code.
> >>
> >> What i meant to say was, set_runtime_config() must have already done this part.
> >
> > On one hand it is true. On the other - *_prep* functions use
> > explicitly passed parameter. I doubt there is a consistency between
> > value in slave config passed via dwc_control and value passed as
> > explicit function parameter.
> 
> I believe it should be consistent.
> 
> @Vinod: Why have we duplicated direction? Once in prep_* and then in
> slave_config?

Documentation and TODO list say that slave_config.direction is kinda
deprecated.

> 
> --
> viresh

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] dw_dmac: check direction properly in dw_dma_cyclic_prep

[viresh kumar]

On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> +static inline bool is_slave_direction(enum dma_transfer_direction direction)
> +{
> +       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
> +}

I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.

Re: [PATCH 1/2] dw_dmac: check direction properly in dw_dma_cyclic_prep

[viresh kumar]

On Thu, Nov 15, 2012 at 8:05 PM, viresh kumar <viresh.kumar@linaro.org> wrote:
> On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
>
>> +static inline bool is_slave_direction(enum dma_transfer_direction direction)
>> +{
>> +       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
>> +}
>
> I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.

Also, what about is_slave_xfer() name?

Re: [PATCH 1/2] dw_dmac: check direction properly in dw_dma_cyclic_prep

[Andy Shevchenko]

On Thu, 2012-11-15 at 20:28 +0530, viresh kumar wrote: 
> On Thu, Nov 15, 2012 at 8:05 PM, viresh kumar <viresh.kumar@linaro.org> wrote:
> > On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> >
> >> +static inline bool is_slave_direction(enum dma_transfer_direction direction)
> >> +{
> >> +       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
> >> +}
> >
> > I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.
> 
> Also, what about is_slave_xfer() name?

No objections.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] dw_dmac: check direction properly in dw_dma_cyclic_prep

[Andy Shevchenko]

On Thu, 2012-11-15 at 20:05 +0530, viresh kumar wrote: 
> On Thu, Nov 15, 2012 at 7:50 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> 
> > +static inline bool is_slave_direction(enum dma_transfer_direction direction)
> > +{
> > +       return (direction == DMA_MEM_TO_DEV) || (direction == DMA_DEV_TO_MEM);
> > +}
> 
> I would add this in dmaengine.h and will also include check for DMA_DEV_TO_DEV.

I briefly checked the other dma drivers and have noticed that dev-2-dev
is not common type of transfer. So, I prefer to keep only those two in
the check function.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy