* [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling
@ 2015-05-01 14:16 Nicholas Mc Guire
2015-05-01 14:16 ` [PATCH 2/3] mtd: sh_flctl: fix alignment of function argument Nicholas Mc Guire
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Nicholas Mc Guire @ 2015-05-01 14:16 UTC (permalink / raw)
To: David Woodhouse
Cc: Brian Norris, Vinod Koul, Laurent Pinchart, Kuninori Morimoto,
Wolfram Sang, Arnd Bergmann, linux-mtd, linux-kernel,
Nicholas Mc Guire
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 <hofrat@osadl.org>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] mtd: sh_flctl: fix alignment of function argument
2015-05-01 14:16 [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Nicholas Mc Guire
@ 2015-05-01 14:16 ` Nicholas Mc Guire
2015-05-01 23:08 ` Laurent Pinchart
2015-05-01 14:16 ` [PATCH 3/3] mtd: sh_flctl: remove unused variable and assignment Nicholas Mc Guire
2015-05-01 23:20 ` [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Laurent Pinchart
2 siblings, 1 reply; 7+ messages in thread
From: Nicholas Mc Guire @ 2015-05-01 14:16 UTC (permalink / raw)
To: David Woodhouse
Cc: Brian Norris, Vinod Koul, Laurent Pinchart, Kuninori Morimoto,
Wolfram Sang, Arnd Bergmann, linux-mtd, linux-kernel,
Nicholas Mc Guire
This just is a minor coding style cleanup - align the function arguments
with the opening (.
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] mtd: sh_flctl: remove unused variable and assignment
2015-05-01 14:16 [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Nicholas Mc Guire
2015-05-01 14:16 ` [PATCH 2/3] mtd: sh_flctl: fix alignment of function argument Nicholas Mc Guire
@ 2015-05-01 14:16 ` Nicholas Mc Guire
2015-05-01 23:07 ` Laurent Pinchart
2015-05-01 23:20 ` [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Laurent Pinchart
2 siblings, 1 reply; 7+ messages in thread
From: Nicholas Mc Guire @ 2015-05-01 14:16 UTC (permalink / raw)
To: David Woodhouse
Cc: Brian Norris, Vinod Koul, Laurent Pinchart, Kuninori Morimoto,
Wolfram Sang, Arnd Bergmann, linux-mtd, linux-kernel,
Nicholas Mc Guire
Fixes a compile warning [-Wunused-but-set-variable] only.
Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] mtd: sh_flctl: remove unused variable and assignment
2015-05-01 14:16 ` [PATCH 3/3] mtd: sh_flctl: remove unused variable and assignment Nicholas Mc Guire
@ 2015-05-01 23:07 ` Laurent Pinchart
0 siblings, 0 replies; 7+ messages in thread
From: Laurent Pinchart @ 2015-05-01 23:07 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: David Woodhouse, Brian Norris, Vinod Koul, Laurent Pinchart,
Kuninori Morimoto, Wolfram Sang, Arnd Bergmann, linux-mtd,
linux-kernel
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 <hofrat@osadl.org>
> ---
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] mtd: sh_flctl: fix alignment of function argument
2015-05-01 14:16 ` [PATCH 2/3] mtd: sh_flctl: fix alignment of function argument Nicholas Mc Guire
@ 2015-05-01 23:08 ` Laurent Pinchart
0 siblings, 0 replies; 7+ messages in thread
From: Laurent Pinchart @ 2015-05-01 23:08 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: David Woodhouse, Brian Norris, Vinod Koul, Laurent Pinchart,
Kuninori Morimoto, Wolfram Sang, Arnd Bergmann, linux-mtd,
linux-kernel
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 <hofrat@osadl.org>
> ---
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling
2015-05-01 14:16 [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Nicholas Mc Guire
2015-05-01 14:16 ` [PATCH 2/3] mtd: sh_flctl: fix alignment of function argument Nicholas Mc Guire
2015-05-01 14:16 ` [PATCH 3/3] mtd: sh_flctl: remove unused variable and assignment Nicholas Mc Guire
@ 2015-05-01 23:20 ` Laurent Pinchart
2015-05-02 6:49 ` Nicholas Mc Guire
2 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2015-05-01 23:20 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: David Woodhouse, Brian Norris, Vinod Koul, Laurent Pinchart,
Kuninori Morimoto, Wolfram Sang, Arnd Bergmann, linux-mtd,
linux-kernel
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 <hofrat@osadl.org>
> ---
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling
2015-05-01 23:20 ` [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Laurent Pinchart
@ 2015-05-02 6:49 ` Nicholas Mc Guire
0 siblings, 0 replies; 7+ messages in thread
From: Nicholas Mc Guire @ 2015-05-02 6:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Nicholas Mc Guire, David Woodhouse, Brian Norris, Vinod Koul,
Laurent Pinchart, Kuninori Morimoto, Wolfram Sang, Arnd Bergmann,
linux-mtd, linux-kernel
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 <hofrat@osadl.org>
> > ---
> >
> > 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-05-02 6:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-01 14:16 [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Nicholas Mc Guire
2015-05-01 14:16 ` [PATCH 2/3] mtd: sh_flctl: fix alignment of function argument Nicholas Mc Guire
2015-05-01 23:08 ` Laurent Pinchart
2015-05-01 14:16 ` [PATCH 3/3] mtd: sh_flctl: remove unused variable and assignment Nicholas Mc Guire
2015-05-01 23:07 ` Laurent Pinchart
2015-05-01 23:20 ` [PATCH 1/3] mtd: sh_flctl: fixup wait_for_completion_timeout return handling Laurent Pinchart
2015-05-02 6:49 ` Nicholas Mc Guire
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).