[PATCH 0/5] mtd: st_spi_fsm: Sweep-up remaining blocking-issues

[PATCH 0/5] mtd: st_spi_fsm: Sweep-up remaining blocking-issues

[Lee Jones]

Brian,

Sincerest apologies for missing these before.

I hope this now covers all of points you raised.

 drivers/mtd/devices/st_spi_fsm.c | 33 +++++++++------------------------
 1 file changed, 9 insertions(+), 24 deletions(-)

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Lee Jones]

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index aefd48d..dcc1dc3 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -785,8 +785,7 @@ static void stfsm_wait_seq(struct stfsm *fsm)
 	dev_err(fsm->dev, "timeout on sequence completion\n");
 }
 
-static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
-			    const uint32_t size)
+static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size)
 {
 	uint32_t remaining = size >> 2;
 	uint32_t avail;
@@ -811,8 +810,7 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
 	}
 }
 
-static int stfsm_write_fifo(struct stfsm *fsm,
-			    const uint32_t *buf, const uint32_t size)
+static int stfsm_write_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size)
 {
 	uint32_t words = size >> 2;
 
@@ -1544,8 +1542,8 @@ static int stfsm_read(struct stfsm *fsm, uint8_t *buf, uint32_t size,
 	return 0;
 }
 
-static int stfsm_write(struct stfsm *fsm, const uint8_t *const buf,
-		       const uint32_t size, const uint32_t offset)
+static int stfsm_write(struct stfsm *fsm, const uint8_t *buf,
+		       uint32_t size, uint32_t offset)
 {
 	struct stfsm_seq *seq = &fsm->stfsm_seq_write;
 	uint32_t data_pads;
@@ -1670,7 +1668,7 @@ static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
 	return 0;
 }
 
-static int stfsm_erase_sector(struct stfsm *fsm, const uint32_t offset)
+static int stfsm_erase_sector(struct stfsm *fsm, uint32_t offset)
 {
 	struct stfsm_seq *seq = &stfsm_seq_erase_sector;
 	int ret;
-- 
1.8.3.2

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Joe Perches]

On Thu, 2014-03-20 at 11:11 +0000, Lee Jones wrote:
> Reported-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Why are these useless?

Even if the object code produced is the same,
these serve as information to the human reader
about how the arguments are used.

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Lee Jones]

On Thu, 20 Mar 2014, Joe Perches wrote:

> On Thu, 2014-03-20 at 11:11 +0000, Lee Jones wrote:
> > Reported-by: Brian Norris <computersforpeace@gmail.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Why are these useless?
> 
> Even if the object code produced is the same,
> these serve as information to the human reader
> about how the arguments are used.

https://lkml.org/lkml/2014/3/20/69

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Joe Perches]

On Thu, 2014-03-20 at 11:41 +0000, Lee Jones wrote:
> On Thu, 20 Mar 2014, Joe Perches wrote:
> 
> > On Thu, 2014-03-20 at 11:11 +0000, Lee Jones wrote:
> > > Reported-by: Brian Norris <computersforpeace@gmail.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 
> > Why are these useless?
> > 
> > Even if the object code produced is the same,
> > these serve as information to the human reader
> > about how the arguments are used.
> 
> https://lkml.org/lkml/2014/3/20/69
> 

>From that email:

suggestion:

	const uint8_t *const buf  => const uint8_t *buf
	const uint32_t size       => uint32_t size
	const uint32_t offset     => uint32_t offset

What was done:

-static int stfsm_write_fifo(struct stfsm *fsm,
-                           const uint32_t *buf, const uint32_t size)
+static int stfsm_write_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size)

Note the removal of the const from uint32_t *buf

Why?

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Brian Norris]

On Thu, Mar 20, 2014 at 04:48:10AM -0700, Joe Perches wrote:
> On Thu, 2014-03-20 at 11:41 +0000, Lee Jones wrote:
> > On Thu, 20 Mar 2014, Joe Perches wrote:
> > > On Thu, 2014-03-20 at 11:11 +0000, Lee Jones wrote:
> > > > Reported-by: Brian Norris <computersforpeace@gmail.com>
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > 
> > > Why are these useless?
> > > 
> > > Even if the object code produced is the same,
> > > these serve as information to the human reader
> > > about how the arguments are used.
> > 
> > https://lkml.org/lkml/2014/3/20/69
> > 
> 
> From that email:
> 
> suggestion:
> 
> 	const uint8_t *const buf  => const uint8_t *buf
> 	const uint32_t size       => uint32_t size
> 	const uint32_t offset     => uint32_t offset
> 
> What was done:
> 
> -static int stfsm_write_fifo(struct stfsm *fsm,
> -                           const uint32_t *buf, const uint32_t size)
> +static int stfsm_write_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size)
> 
> Note the removal of the const from uint32_t *buf

Good catch. That wasn't my intention.

> Why?

I can let Lee speak for himself. But I'm thinking I'll squash this diff
in, if no one minds:

Signed-off-by: Brian Norris <computersforpeace@gmail.com>

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index bea1416aad07..1957d7c8e185 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -810,7 +810,8 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size)
 	}
 }
 
-static int stfsm_write_fifo(struct stfsm *fsm, uint32_t *buf, uint32_t size)
+static int stfsm_write_fifo(struct stfsm *fsm, const uint32_t *buf,
+			    uint32_t size)
 {
 	uint32_t words = size >> 2;
 
@@ -1806,7 +1807,7 @@ out1:
 	return ret;
 }
 
-static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
+static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
 	uint32_t tmp[2];

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Lee Jones]

> Good catch. That wasn't my intention.

+1

> > Why?

An oversight.

> I can let Lee speak for himself. But I'm thinking I'll squash this diff
> in, if no one minds:
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Patch looks good to me:
  Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Brian Norris]

On Thu, Mar 20, 2014 at 5:03 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>
> Patch looks good to me:
>   Acked-by: Lee Jones <lee.jones@linaro.org>

Squashed into $SUBJECT patch and pushed to l2-mtd.git.

Brian

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Joe Perches]

On Thu, 2014-03-20 at 12:03 +0000, Lee Jones wrote:
> > Good catch. That wasn't my intention.
> > > Why?
> An oversight.

That's still not an explanation.

Why, unless cast away by the code itself, is
const removal a good thing?

It does serve as an indication to a reader what
the code does with the argument.

About the only reason I can think of arguing in
favor of removal is inconsistent application of
const within the module.

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Brian Norris]

On Thu, Mar 20, 2014 at 5:13 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2014-03-20 at 12:03 +0000, Lee Jones wrote:
>> > Good catch. That wasn't my intention.
>> > > Why?
>> An oversight.
>
> That's still not an explanation.
>
> Why, unless cast away by the code itself, is
> const removal a good thing?

It's not so much removal as it is review of the initial driver merge.
I'd contend that const was applied somewhat thoughtlessly originally,
and it didn't really serve a good purpose.

> It does serve as an indication to a reader what
> the code does with the argument.
>
> About the only reason I can think of arguing in
> favor of removal is inconsistent application of
> const within the module.

That's one good reason. And not only consistency within the modules,
but consistency within the subsystem (and the kernel at large,
really). There's rarely a case of a const function parameter. And I'm
sure there are numerous function parameters which could potentially be
marked 'const'.

I also don't think that a function parameter is the right place to
mark const like this. Function arguments are always pass-by-value, so
this 'const' tells users (callers) nothing useful. It only provides
useless constraints on what the function can do with its copy of the
parameter.

Brian

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Joe Perches]

On Thu, 2014-03-20 at 05:41 -0700, Brian Norris wrote:
> I also don't think that a function parameter is the right place to
> mark const like this. Function arguments are always pass-by-value, so
> this 'const' tells users (callers) nothing useful. It only provides
> useless constraints on what the function can do with its copy of the
> parameter.

Again, that's not useless information.

And as you've seen, just making these changes
can be error prone.

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Brian Norris]

On Thu, Mar 20, 2014 at 05:44:45AM -0700, Joe Perches wrote:
> On Thu, 2014-03-20 at 05:41 -0700, Brian Norris wrote:
> > I also don't think that a function parameter is the right place to
> > mark const like this. Function arguments are always pass-by-value, so
> > this 'const' tells users (callers) nothing useful. It only provides
> > useless constraints on what the function can do with its copy of the
> > parameter.
> 
> Again, that's not useless information.

For local, pass-by-value function arguments (i.e., constant data, or
constant pointers to data), I respectfully disagree. (For "pointers to
constant data", I completely agree that the 'const' info is useful.)

> And as you've seen, just making these changes
> can be error prone.

Thank you for catching our mistake now. But I don't think that is
relevant; just because you caught an error doesn't mean that the change
(primarily for consistency's sake) should be avoided entirely.

Unless you have a more convincing argument, this code will remain as-is.

Regards,
Brian

[PATCH 1/5] mtd: st_spi_fsm: Remove useless consts from function arguments

[Lee Jones]

> > > Good catch. That wasn't my intention.
> > > > Why?
> > An oversight.
> 
> That's still not an explanation.
> 
> Why, unless cast away by the code itself, is
> const removal a good thing?
> 
> It does serve as an indication to a reader what
> the code does with the argument.
> 
> About the only reason I can think of arguing in
> favor of removal is inconsistent application of
> const within the module.

Your qualm is not with me in this instance. I don't ever mind holding
my hands up to mistakes I make or standing up for decisions I believe
in, but in this case I'm merely satisfying the wishes of a maintainer
I'm submitting code to.

I am strongly un-opinionated about these particular changes.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/5] mtd: st_spi_fsm: Avoid duplicating MTD core code

[Lee Jones]

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index dcc1dc3..27f9ec9 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -1728,14 +1728,6 @@ static int stfsm_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 	dev_dbg(fsm->dev, "%s to 0x%08x, len %zd\n", __func__, (u32)to, len);
 
-	*retlen = 0;
-
-	if (!len)
-		return 0;
-
-	if (to + len > mtd->size)
-		return -EINVAL;
-
 	/* Offset within page */
 	page_offs = to % FLASH_PAGESIZE;
 
-- 
1.8.3.2

[PATCH 3/5] mtd: st_spi_fsm: Correct vendor name spelling issue - missing "M"

[Lee Jones]

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 27f9ec9..9de753d 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -3,7 +3,7 @@
  *
  * Author: Angus Clark <angus.clark@st.com>
  *
- * Copyright (C) 2010-2014 STicroelectronics Limited
+ * Copyright (C) 2010-2014 STMicroelectronics Limited
  *
  * JEDEC probe based on drivers/mtd/devices/m25p80.c
  *
-- 
1.8.3.2

[PATCH 4/5] mtd: st_spi_fsm: Allow loop to run at least once before giving up CPU

[Lee Jones]

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 9de753d..921b44a 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -860,8 +860,6 @@ static uint8_t stfsm_wait_busy(struct stfsm *fsm)
 	 */
 	deadline = jiffies + FLASH_MAX_BUSY_WAIT;
 	while (!timeout) {
-		cond_resched();
-
 		if (time_after_eq(jiffies, deadline))
 			timeout = 1;
 
@@ -880,6 +878,8 @@ static uint8_t stfsm_wait_busy(struct stfsm *fsm)
 		if (!timeout)
 			/* Restart */
 			writel(seq->seq_cfg, fsm->base + SPI_FAST_SEQ_CFG);
+
+		cond_resched();
 	}
 
 	dev_err(fsm->dev, "timeout on wait_busy\n");
-- 
1.8.3.2

[PATCH 5/5] mtd: st_spi_fsm: Succinctly reorganise .remove()

[Lee Jones]

Reported-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 921b44a..bea1416 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -2081,13 +2081,8 @@ static int stfsm_probe(struct platform_device *pdev)
 static int stfsm_remove(struct platform_device *pdev)
 {
 	struct stfsm *fsm = platform_get_drvdata(pdev);
-	int err;
 
-	err = mtd_device_unregister(&fsm->mtd);
-	if (err)
-		return err;
-
-	return 0;
+	return mtd_device_unregister(&fsm->mtd);
 }
 
 static struct of_device_id stfsm_match[] = {
-- 
1.8.3.2

[PATCH 0/5] mtd: st_spi_fsm: Sweep-up remaining blocking-issues

[Brian Norris]

On Thu, Mar 20, 2014 at 11:11:42AM +0000, Lee Jones wrote:
> Sincerest apologies for missing these before.
> 
> I hope this now covers all of points you raised.

Looks good. Thanks for following up. Pushed all to l2-mtd.git.

Thanks,
Brian