wait_for_completion_timeout() returns unsigned long not int so the check
for <= should be == and the type unsigned long. This fixes up the return
value handling and returns -ETIMEDOUT on timeout rather than 0 and 1 on
on success rather than a more or less random remaining number of jiffies.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
call sites:
read_fiforeg,write_ec_fiforeg assume > 0 == success
and the comment in flctl_dma_fifo0_transfe states
/* ret > 0 is success */
return ret;
since it is only checking for > 0 in the call-sites
returning -ETIMEDOUT should be fine.
Patch was compile tested with ap325rxa_defconfig (implies
CONFIG_MTD_NAND_SH_FLCTL=y)
Patch is against 4.1-rc1 (localversion-next is -next-20150501)
drivers/mtd/nand/sh_flctl.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index c3ce81c..4450864 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -354,6 +354,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
dma_cookie_t cookie = -EINVAL;
uint32_t reg;
int ret;
+ unsigned long time_left;
if (dir == DMA_FROM_DEVICE) {
chan = flctl->chan_fifo0_rx;
@@ -388,14 +389,16 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
goto out;
}
- ret =
+ time_left =
wait_for_completion_timeout(&flctl->dma_complete,
msecs_to_jiffies(3000));
- if (ret <= 0) {
+ if (time_left == 0) {
dmaengine_terminate_all(chan);
dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
- }
+ ret = -ETIMEDOUT;
+ } else
+ ret = 1; /* completion succeeded */
out:
reg = readl(FLINTDMACR(flctl));
--
1.7.10.4
This just is a minor coding style cleanup - align the function arguments
with the opening (.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
Patch was compile tested with ap325rxa_defconfig (implies
CONFIG_MTD_NAND_SH_FLCTL=y)
Patch is against 4.1-rc1 (localversion-next is -next-20150501)
drivers/mtd/nand/sh_flctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index 4450864..b9f265a 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -391,7 +391,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
time_left =
wait_for_completion_timeout(&flctl->dma_complete,
- msecs_to_jiffies(3000));
+ msecs_to_jiffies(3000));
if (time_left == 0) {
dmaengine_terminate_all(chan);
--
1.7.10.4
Fixes a compile warning [-Wunused-but-set-variable] only.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
This fixes the compile time warning:
drivers/mtd/nand/sh_flctl.c: In function 'flctl_dma_fifo0_transfer':
drivers/mtd/nand/sh_flctl.c:354:15: warning: variable 'cookie' set but not
used [-Wunused-but-set-variable]
as dmaengine_submit only reads and returns desc->tx_submit(desc) which
is then unused in flctl_dma_fifo0_transfer, this should be side-effect
free to remove.
Patch was compile tested with ap325rxa_defconfig (implies
CONFIG_MTD_NAND_SH_FLCTL=y)
Patch is against 4.1-rc1 (localversion-next is -next-20150501)
drivers/mtd/nand/sh_flctl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
index b9f265a..6a5c4f7 100644
--- a/drivers/mtd/nand/sh_flctl.c
+++ b/drivers/mtd/nand/sh_flctl.c
@@ -351,7 +351,6 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
struct dma_chan *chan;
enum dma_transfer_direction tr_dir;
dma_addr_t dma_addr;
- dma_cookie_t cookie = -EINVAL;
uint32_t reg;
int ret;
unsigned long time_left;
@@ -377,7 +376,6 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl *flctl, unsigned long *buf,
desc->callback = flctl_dma_complete;
desc->callback_param = flctl;
- cookie = dmaengine_submit(desc);
dma_async_issue_pending(chan);
} else {
--
1.7.10.4
Hi Nicholas,
Thank you for the patch.
On Friday 01 May 2015 16:16:03 Nicholas Mc Guire wrote:
> Fixes a compile warning [-Wunused-but-set-variable] only.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> ---
>
> This fixes the compile time warning:
> drivers/mtd/nand/sh_flctl.c: In function 'flctl_dma_fifo0_transfer':
> drivers/mtd/nand/sh_flctl.c:354:15: warning: variable 'cookie' set but not
> used [-Wunused-but-set-variable]
>
> as dmaengine_submit only reads and returns desc->tx_submit(desc) which
> is then unused in flctl_dma_fifo0_transfer, this should be side-effect
> free to remove.
You can ignore its return value, but you can't remove the call to
dmaengine_submit, as it's part of the DMA engine API and desc->tx_submit()
does important work.
> Patch was compile tested with ap325rxa_defconfig (implies
> CONFIG_MTD_NAND_SH_FLCTL=y)
>
> Patch is against 4.1-rc1 (localversion-next is -next-20150501)
>
> drivers/mtd/nand/sh_flctl.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index b9f265a..6a5c4f7 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -351,7 +351,6 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf, struct dma_chan *chan;
> enum dma_transfer_direction tr_dir;
> dma_addr_t dma_addr;
> - dma_cookie_t cookie = -EINVAL;
> uint32_t reg;
> int ret;
> unsigned long time_left;
> @@ -377,7 +376,6 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf,
>
> desc->callback = flctl_dma_complete;
> desc->callback_param = flctl;
> - cookie = dmaengine_submit(desc);
>
> dma_async_issue_pending(chan);
> } else {
--
Regards,
Laurent Pinchart
Hi Nicholas,
Thank you for the patch.
On Friday 01 May 2015 16:16:02 Nicholas Mc Guire wrote:
> This just is a minor coding style cleanup - align the function arguments
> with the opening (.
I would just squash this into patch 1/3.
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> ---
>
> Patch was compile tested with ap325rxa_defconfig (implies
> CONFIG_MTD_NAND_SH_FLCTL=y)
>
> Patch is against 4.1-rc1 (localversion-next is -next-20150501)
>
> drivers/mtd/nand/sh_flctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 4450864..b9f265a 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -391,7 +391,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf,
>
> time_left =
> wait_for_completion_timeout(&flctl->dma_complete,
> - msecs_to_jiffies(3000));
> + msecs_to_jiffies(3000));
>
> if (time_left == 0) {
> dmaengine_terminate_all(chan);
--
Regards,
Laurent Pinchart
Hi Nicholas,
Thank you for the patch.
On Friday 01 May 2015 16:16:01 Nicholas Mc Guire wrote:
> wait_for_completion_timeout() returns unsigned long not int so the check
> for <= should be == and the type unsigned long. This fixes up the return
> value handling and returns -ETIMEDOUT on timeout rather than 0 and 1 on
> on success rather than a more or less random remaining number of jiffies.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> ---
>
> call sites:
> read_fiforeg,write_ec_fiforeg assume > 0 == success
> and the comment in flctl_dma_fifo0_transfe states
> /* ret > 0 is success */
> return ret;
> since it is only checking for > 0 in the call-sites
> returning -ETIMEDOUT should be fine.
>
> Patch was compile tested with ap325rxa_defconfig (implies
> CONFIG_MTD_NAND_SH_FLCTL=y)
>
> Patch is against 4.1-rc1 (localversion-next is -next-20150501)
>
> drivers/mtd/nand/sh_flctl.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index c3ce81c..4450864 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -354,6 +354,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf, dma_cookie_t cookie = -EINVAL;
> uint32_t reg;
> int ret;
> + unsigned long time_left;
>
> if (dir == DMA_FROM_DEVICE) {
> chan = flctl->chan_fifo0_rx;
> @@ -388,14 +389,16 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> *flctl, unsigned long *buf, goto out;
> }
>
> - ret =
> + time_left =
> wait_for_completion_timeout(&flctl->dma_complete,
> msecs_to_jiffies(3000));
>
> - if (ret <= 0) {
> + if (time_left == 0) {
> dmaengine_terminate_all(chan);
> dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
> - }
> + ret = -ETIMEDOUT;
> + } else
> + ret = 1; /* completion succeeded */
I'd go one step further and make this function return < 0 on error and 0 on
success, like usually done through the kernel API. You could then simplify the
code using something like
if (!wait_for_completion_timeout(&flctl->dma_complete,
msecs_to_jiffies(3000))) {
dmaengine_terminate_all(chan);
dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
ret = -ETIMEDOUT;
}
(pre-initializing ret to 0)
>
> out:
> reg = readl(FLINTDMACR(flctl));
--
Regards,
Laurent Pinchart
On Sat, 02 May 2015, Laurent Pinchart wrote:
> Hi Nicholas,
>
> Thank you for the patch.
>
> On Friday 01 May 2015 16:16:01 Nicholas Mc Guire wrote:
> > wait_for_completion_timeout() returns unsigned long not int so the check
> > for <= should be == and the type unsigned long. This fixes up the return
> > value handling and returns -ETIMEDOUT on timeout rather than 0 and 1 on
> > on success rather than a more or less random remaining number of jiffies.
> >
> > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > ---
> >
> > call sites:
> > read_fiforeg,write_ec_fiforeg assume > 0 == success
> > and the comment in flctl_dma_fifo0_transfe states
> > /* ret > 0 is success */
> > return ret;
> > since it is only checking for > 0 in the call-sites
> > returning -ETIMEDOUT should be fine.
> >
> > Patch was compile tested with ap325rxa_defconfig (implies
> > CONFIG_MTD_NAND_SH_FLCTL=y)
> >
> > Patch is against 4.1-rc1 (localversion-next is -next-20150501)
> >
> > drivers/mtd/nand/sh_flctl.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> > index c3ce81c..4450864 100644
> > --- a/drivers/mtd/nand/sh_flctl.c
> > +++ b/drivers/mtd/nand/sh_flctl.c
> > @@ -354,6 +354,7 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> > *flctl, unsigned long *buf, dma_cookie_t cookie = -EINVAL;
> > uint32_t reg;
> > int ret;
> > + unsigned long time_left;
> >
> > if (dir == DMA_FROM_DEVICE) {
> > chan = flctl->chan_fifo0_rx;
> > @@ -388,14 +389,16 @@ static int flctl_dma_fifo0_transfer(struct sh_flctl
> > *flctl, unsigned long *buf, goto out;
> > }
> >
> > - ret =
> > + time_left =
> > wait_for_completion_timeout(&flctl->dma_complete,
> > msecs_to_jiffies(3000));
> >
> > - if (ret <= 0) {
> > + if (time_left == 0) {
> > dmaengine_terminate_all(chan);
> > dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
> > - }
> > + ret = -ETIMEDOUT;
> > + } else
> > + ret = 1; /* completion succeeded */
>
> I'd go one step further and make this function return < 0 on error and 0 on
> success, like usually done through the kernel API. You could then simplify the
> code using something like
>
> if (!wait_for_completion_timeout(&flctl->dma_complete,
> msecs_to_jiffies(3000))) {
> dmaengine_terminate_all(chan);
> dev_err(&flctl->pdev->dev, "wait_for_completion_timeout\n");
> ret = -ETIMEDOUT;
> }
>
> (pre-initializing ret to 0)
>
> >
> > out:
> > reg = readl(FLINTDMACR(flctl));
>
ok - given that this needs some fixup anyway due to
not properly reading dmaengine_submit() - I'll fix
things up and re-submit.
thx!
hofrat