[PATCH 0/5] sf: Cleanup

[PATCH 0/5] sf: Cleanup

[Jagan Teki]

Cleanup of SF, no precise functionality changes. 

Any inputs?
Jagan.

Jagan Teki (5):
  mtd: spi: Call sst_write in _write ops
  cmd: sf Drop reassignment of new into flash
  env: sf: Preserve and free the previous flash
  mtd: sf: Drop plat from sf_probe
  mtd: spi: Use IS_ENABLED to prevent ifdef

 cmd/sf.c                       |  3 ---
 drivers/mtd/spi/sf_internal.h  | 10 ++++++++++
 drivers/mtd/spi/sf_probe.c     | 19 ++++++++-----------
 drivers/mtd/spi/spi-nor-core.c | 13 +++++++------
 env/sf.c                       | 18 ++++++++++--------
 5 files changed, 35 insertions(+), 28 deletions(-)

-- 
2.20.1

[PATCH 1/5] mtd: spi: Call sst_write in _write ops

[Jagan Teki]

Currently spi-nor code is assigning _write ops for SST
and other flashes separately.?

Just call the sst_write from generic write ops and return
if SST flash found, this way it avoids the confusion of
multiple write ops assignment during the scan and makes
it more feasible for code readability.

No functionality changes.

Cc: Simon Glass <sjg@chromium.org>
Cc: Vignesh R <vigneshr@ti.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/mtd/spi/spi-nor-core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 3d4361493e..984cece0b0 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1233,6 +1233,12 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	size_t page_offset, page_remain, i;
 	ssize_t ret;
 
+#ifdef CONFIG_SPI_FLASH_SST
+	/* sst nor chips use AAI word program */
+	if (nor->info->flags & SST_WRITE)
+		return sst_write(mtd, to, len, retlen, buf);
+#endif
+
 	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
 
 	if (!len)
@@ -2528,6 +2534,7 @@ int spi_nor_scan(struct spi_nor *nor)
 	mtd->size = params.size;
 	mtd->_erase = spi_nor_erase;
 	mtd->_read = spi_nor_read;
+	mtd->_write = spi_nor_write;
 
 #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
 	/* NOR protection support for STmicro/Micron chips and similar */
@@ -2551,13 +2558,7 @@ int spi_nor_scan(struct spi_nor *nor)
 		nor->flash_unlock = sst26_unlock;
 		nor->flash_is_locked = sst26_is_locked;
 	}
-
-	/* sst nor chips use AAI word program */
-	if (info->flags & SST_WRITE)
-		mtd->_write = sst_write;
-	else
 #endif
-		mtd->_write = spi_nor_write;
 
 	if (info->flags & USE_FSR)
 		nor->flags |= SNOR_F_USE_FSR;
-- 
2.20.1

[PATCH 1/5] mtd: spi: Call sst_write in _write ops

[Jagan Teki]

On Thu, May 14, 2020 at 5:41 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Currently spi-nor code is assigning _write ops for SST
> and other flashes separately.
>
> Just call the sst_write from generic write ops and return
> if SST flash found, this way it avoids the confusion of
> multiple write ops assignment during the scan and makes
> it more feasible for code readability.
>
> No functionality changes.
>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Vignesh R <vigneshr@ti.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---

Applied to u-boot-spi/master

[PATCH 2/5] cmd: sf Drop reassignment of new into flash

[Jagan Teki]

The new pointer points to flash found and that would
assign it to global 'flash' pointer for further flash
operations and also keep track of old flash pointer.

This would happen if the probe is successful or even
failed, but current code assigning new into flash before
and after checking the new.

So, drop the assignment after new checks so flash always
latest new pointer even if probe failed or succeed.

Cc: Simon Glass <sjg@chromium.org>
Cc: Vignesh R <vigneshr@ti.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 cmd/sf.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index e993b3e5ad..302201c2b0 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -141,13 +141,10 @@ static int do_spi_flash_probe(int argc, char * const argv[])
 
 	new = spi_flash_probe(bus, cs, speed, mode);
 	flash = new;
-
 	if (!new) {
 		printf("Failed to initialize SPI flash at %u:%u\n", bus, cs);
 		return 1;
 	}
-
-	flash = new;
 #endif
 
 	return 0;
-- 
2.20.1

[PATCH 2/5] cmd: sf Drop reassignment of new into flash

[Jagan Teki]

On Thu, May 14, 2020 at 5:42 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> The new pointer points to flash found and that would
> assign it to global 'flash' pointer for further flash
> operations and also keep track of old flash pointer.
>
> This would happen if the probe is successful or even
> failed, but current code assigning new into flash before
> and after checking the new.
>
> So, drop the assignment after new checks so flash always
> latest new pointer even if probe failed or succeed.
>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Vignesh R <vigneshr@ti.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---

Applied to u-boot-spi/master

[PATCH 3/5] env: sf: Preserve and free the previous flash

[Jagan Teki]

env_flash is a global flash pointer, and the probe would
happen only if env_flash is NULL, but there is no checking
and free the pointer if is not NULL.

So, this patch frees the env_flash if it's not NULL, and
get the probed flash in new flash pointer and finally
assign into env_flash.

Note: Similar approach has been followed and tested in
cmd/sf.c

Cc: Simon Glass <sjg@chromium.org>
Cc: Vignesh R <vigneshr@ti.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 env/sf.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index 64c57f2cdf..af59c8375c 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -50,15 +50,17 @@ static int setup_flash_device(void)
 
 	env_flash = dev_get_uclass_priv(new);
 #else
+	struct spi_flash *new;
 
-	if (!env_flash) {
-		env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
-			CONFIG_ENV_SPI_CS,
-			CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
-		if (!env_flash) {
-			env_set_default("spi_flash_probe() failed", 0);
-			return -EIO;
-		}
+	if (env_flash)
+		spi_flash_free(env_flash);
+
+	new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
+			      CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
+	env_flash = new;
+	if (!new) {
+		env_set_default("spi_flash_probe() failed", 0);
+		return -EIO;
 	}
 #endif
 	return 0;
-- 
2.20.1

[PATCH 3/5] env: sf: Preserve and free the previous flash

[Pratyush Yadav]

Hi Jagan,

On 14/05/20 05:41PM, Jagan Teki wrote:
> env_flash is a global flash pointer, and the probe would
> happen only if env_flash is NULL, but there is no checking
> and free the pointer if is not NULL.
> 
> So, this patch frees the env_flash if it's not NULL, and
> get the probed flash in new flash pointer and finally
> assign into env_flash.
> 
> Note: Similar approach has been followed and tested in
> cmd/sf.c
> 
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Vignesh R <vigneshr@ti.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  env/sf.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index 64c57f2cdf..af59c8375c 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -50,15 +50,17 @@ static int setup_flash_device(void)
>  
>  	env_flash = dev_get_uclass_priv(new);
>  #else
> +	struct spi_flash *new;
>  
> -	if (!env_flash) {
> -		env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> -			CONFIG_ENV_SPI_CS,
> -			CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> -		if (!env_flash) {
> -			env_set_default("spi_flash_probe() failed", 0);
> -			return -EIO;
> -		}
> +	if (env_flash)
> +		spi_flash_free(env_flash);
> +
> +	new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,

Why not assign env_flash directly here? You do it in the very next 
statement anyway. I don't think the extra 'new' variable is needed.

> +			      CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> +	env_flash = new;
> +	if (!new) {
> +		env_set_default("spi_flash_probe() failed", 0);
> +		return -EIO;
>  	}
>  #endif
>  	return 0;
> -- 
> 2.20.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 3/5] env: sf: Preserve and free the previous flash

[Jagan Teki]

On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Jagan,
>
> On 14/05/20 05:41PM, Jagan Teki wrote:
> > env_flash is a global flash pointer, and the probe would
> > happen only if env_flash is NULL, but there is no checking
> > and free the pointer if is not NULL.
> >
> > So, this patch frees the env_flash if it's not NULL, and
> > get the probed flash in new flash pointer and finally
> > assign into env_flash.
> >
> > Note: Similar approach has been followed and tested in
> > cmd/sf.c
> >
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Vignesh R <vigneshr@ti.com>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  env/sf.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/env/sf.c b/env/sf.c
> > index 64c57f2cdf..af59c8375c 100644
> > --- a/env/sf.c
> > +++ b/env/sf.c
> > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> >
> >       env_flash = dev_get_uclass_priv(new);
> >  #else
> > +     struct spi_flash *new;
> >
> > -     if (!env_flash) {
> > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > -                     CONFIG_ENV_SPI_CS,
> > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > -             if (!env_flash) {
> > -                     env_set_default("spi_flash_probe() failed", 0);
> > -                     return -EIO;
> > -             }
> > +     if (env_flash)
> > +             spi_flash_free(env_flash);
> > +
> > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
>
> Why not assign env_flash directly here? You do it in the very next
> statement anyway. I don't think the extra 'new' variable is needed.

If flash pointer is used free it, before probing a new flash and
storing it in flash. In this scenario it requires local new pointer,
it was known observation we have faced in cmd/sf.c

Jagan.

[PATCH 3/5] env: sf: Preserve and free the previous flash

[Pratyush Yadav]

On 15/05/20 01:14PM, Jagan Teki wrote:
> On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > Hi Jagan,
> >
> > On 14/05/20 05:41PM, Jagan Teki wrote:
> > > env_flash is a global flash pointer, and the probe would
> > > happen only if env_flash is NULL, but there is no checking
> > > and free the pointer if is not NULL.
> > >
> > > So, this patch frees the env_flash if it's not NULL, and
> > > get the probed flash in new flash pointer and finally
> > > assign into env_flash.
> > >
> > > Note: Similar approach has been followed and tested in
> > > cmd/sf.c
> > >
> > > Cc: Simon Glass <sjg@chromium.org>
> > > Cc: Vignesh R <vigneshr@ti.com>
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  env/sf.c | 18 ++++++++++--------
> > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/env/sf.c b/env/sf.c
> > > index 64c57f2cdf..af59c8375c 100644
> > > --- a/env/sf.c
> > > +++ b/env/sf.c
> > > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> > >
> > >       env_flash = dev_get_uclass_priv(new);
> > >  #else
> > > +     struct spi_flash *new;
> > >
> > > -     if (!env_flash) {
> > > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > > -                     CONFIG_ENV_SPI_CS,
> > > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > > -             if (!env_flash) {
> > > -                     env_set_default("spi_flash_probe() failed", 0);
> > > -                     return -EIO;
> > > -             }
> > > +     if (env_flash)
> > > +             spi_flash_free(env_flash);
> > > +
> > > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
> >
> > Why not assign env_flash directly here? You do it in the very next
> > statement anyway. I don't think the extra 'new' variable is needed.
> 
> If flash pointer is used free it, before probing a new flash and
> storing it in flash.

Understood.

> In this scenario it requires local new pointer,
> it was known observation we have faced in cmd/sf.c
 
What difference does using the local pointer make though? We don't do 
anything with it. We just assign it to env_flash in the next statement. 
This is similar to doing:

  a = foo();
  b = a;
  if (a)
  	...

The below snippet is equivalent:

  b = foo();
  if (b)
  	...

We can get rid of 'a' this way.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 3/5] env: sf: Preserve and free the previous flash

[Jagan Teki]

On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> On 15/05/20 01:14PM, Jagan Teki wrote:
> > On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > Hi Jagan,
> > >
> > > On 14/05/20 05:41PM, Jagan Teki wrote:
> > > > env_flash is a global flash pointer, and the probe would
> > > > happen only if env_flash is NULL, but there is no checking
> > > > and free the pointer if is not NULL.
> > > >
> > > > So, this patch frees the env_flash if it's not NULL, and
> > > > get the probed flash in new flash pointer and finally
> > > > assign into env_flash.
> > > >
> > > > Note: Similar approach has been followed and tested in
> > > > cmd/sf.c
> > > >
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Cc: Vignesh R <vigneshr@ti.com>
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > >  env/sf.c | 18 ++++++++++--------
> > > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/env/sf.c b/env/sf.c
> > > > index 64c57f2cdf..af59c8375c 100644
> > > > --- a/env/sf.c
> > > > +++ b/env/sf.c
> > > > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> > > >
> > > >       env_flash = dev_get_uclass_priv(new);
> > > >  #else
> > > > +     struct spi_flash *new;
> > > >
> > > > -     if (!env_flash) {
> > > > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > > > -                     CONFIG_ENV_SPI_CS,
> > > > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > > > -             if (!env_flash) {
> > > > -                     env_set_default("spi_flash_probe() failed", 0);
> > > > -                     return -EIO;
> > > > -             }
> > > > +     if (env_flash)
> > > > +             spi_flash_free(env_flash);
> > > > +
> > > > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
> > >
> > > Why not assign env_flash directly here? You do it in the very next
> > > statement anyway. I don't think the extra 'new' variable is needed.
> >
> > If flash pointer is used free it, before probing a new flash and
> > storing it in flash.
>
> Understood.
>
> > In this scenario it requires local new pointer,
> > it was known observation we have faced in cmd/sf.c
>
> What difference does using the local pointer make though? We don't do
> anything with it. We just assign it to env_flash in the next statement.
> This is similar to doing:
>
>   a = foo();
>   b = a;
>   if (a)
>         ...
>
> The below snippet is equivalent:
>
>   b = foo();
>   if (b)
>         ...
>
> We can get rid of 'a' this way.

The scenario here, seems different here we need to take care of free
the old pointer and update new pointer with a global pointer like
below.

if (b)
    free(b);
a = foo();
b = a;
if (!a)
   fail;

So, you mean to say as below?

if (b)
    free(b);
b = foo();
if (!b)
     fail;

Jagan.

[PATCH 3/5] env: sf: Preserve and free the previous flash

[Pratyush Yadav]

On 18/05/20 05:36PM, Jagan Teki wrote:
> On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > On 15/05/20 01:14PM, Jagan Teki wrote:
> > > On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> > > >
> > > > Hi Jagan,
> > > >
> > > > On 14/05/20 05:41PM, Jagan Teki wrote:
> > > > > env_flash is a global flash pointer, and the probe would
> > > > > happen only if env_flash is NULL, but there is no checking
> > > > > and free the pointer if is not NULL.
> > > > >
> > > > > So, this patch frees the env_flash if it's not NULL, and
> > > > > get the probed flash in new flash pointer and finally
> > > > > assign into env_flash.
> > > > >
> > > > > Note: Similar approach has been followed and tested in
> > > > > cmd/sf.c
> > > > >
> > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > Cc: Vignesh R <vigneshr@ti.com>
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > > >  env/sf.c | 18 ++++++++++--------
> > > > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/env/sf.c b/env/sf.c
> > > > > index 64c57f2cdf..af59c8375c 100644
> > > > > --- a/env/sf.c
> > > > > +++ b/env/sf.c
> > > > > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> > > > >
> > > > >       env_flash = dev_get_uclass_priv(new);
> > > > >  #else
> > > > > +     struct spi_flash *new;
> > > > >
> > > > > -     if (!env_flash) {
> > > > > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > > > > -                     CONFIG_ENV_SPI_CS,
> > > > > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > > > > -             if (!env_flash) {
> > > > > -                     env_set_default("spi_flash_probe() failed", 0);
> > > > > -                     return -EIO;
> > > > > -             }
> > > > > +     if (env_flash)
> > > > > +             spi_flash_free(env_flash);
> > > > > +
> > > > > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
> > > >
> > > > Why not assign env_flash directly here? You do it in the very next
> > > > statement anyway. I don't think the extra 'new' variable is needed.
> > >
> > > If flash pointer is used free it, before probing a new flash and
> > > storing it in flash.
> >
> > Understood.
> >
> > > In this scenario it requires local new pointer,
> > > it was known observation we have faced in cmd/sf.c
> >
> > What difference does using the local pointer make though? We don't do
> > anything with it. We just assign it to env_flash in the next statement.
> > This is similar to doing:
> >
> >   a = foo();
> >   b = a;
> >   if (a)
> >         ...
> >
> > The below snippet is equivalent:
> >
> >   b = foo();
> >   if (b)
> >         ...
> >
> > We can get rid of 'a' this way.
> 
> The scenario here, seems different here we need to take care of free
> the old pointer and update new pointer with a global pointer like
> below.
> 
> if (b)
>     free(b);
> a = foo();
> b = a;
> if (!a)
>    fail;
> 
> So, you mean to say as below?
> 
> if (b)
>     free(b);
> b = foo();
> if (!b)
>      fail;

Yes, I mean this. Since the pointer b is freed/unassigned, the old 
pointer value is of no use to us. Getting rid of a (or new in case of 
this patch) makes the code a bit cleaner.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 4/5] mtd: sf: Drop plat from sf_probe

[Jagan Teki]

dm_spi_slave_platdata used in sf_probe for printing
plat->cs value and there is no relevant usage apart
from this.

We have enouch debug messages available in SPI and SF
areas so drop this plat get and associated bug statement.

Cc: Simon Glass <sjg@chromium.org>
Cc: Vignesh R <vigneshr@ti.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/mtd/spi/sf_probe.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
index 72b6ee702d..89e384901c 100644
--- a/drivers/mtd/spi/sf_probe.c
+++ b/drivers/mtd/spi/sf_probe.c
@@ -140,13 +140,11 @@ static int spi_flash_std_get_sw_write_prot(struct udevice *dev)
 int spi_flash_std_probe(struct udevice *dev)
 {
 	struct spi_slave *slave = dev_get_parent_priv(dev);
-	struct dm_spi_slave_platdata *plat = dev_get_parent_platdata(dev);
 	struct spi_flash *flash;
 
 	flash = dev_get_uclass_priv(dev);
 	flash->dev = dev;
 	flash->spi = slave;
-	debug("%s: slave=%p, cs=%d\n", __func__, slave, plat->cs);
 	return spi_flash_probe_slave(flash);
 }
 
-- 
2.20.1

[PATCH 4/5] mtd: sf: Drop plat from sf_probe

[Pratyush Yadav]

On 14/05/20 05:41PM, Jagan Teki wrote:
> dm_spi_slave_platdata used in sf_probe for printing
> plat->cs value and there is no relevant usage apart
> from this.
> 
> We have enouch debug messages available in SPI and SF

s/enouch/enough/

> areas so drop this plat get and associated bug statement.
> 
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Vignesh R <vigneshr@ti.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 4/5] mtd: sf: Drop plat from sf_probe

[Jagan Teki]

On Fri, May 15, 2020 at 12:47 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> On 14/05/20 05:41PM, Jagan Teki wrote:
> > dm_spi_slave_platdata used in sf_probe for printing
> > plat->cs value and there is no relevant usage apart
> > from this.
> >
> > We have enouch debug messages available in SPI and SF
>
> s/enouch/enough/

Applied to u-boot-spi/master

[PATCH 5/5] mtd: spi: Use IS_ENABLED to prevent ifdef

[Jagan Teki]

Use IS_ENABLED to prevent ifdef in sf_probe.c

Cc: Simon Glass <sjg@chromium.org>
Cc: Vignesh R <vigneshr@ti.com>
Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/mtd/spi/sf_internal.h | 10 ++++++++++
 drivers/mtd/spi/sf_probe.c    | 17 ++++++++---------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
index 940b2e4c9e..544ed74a5f 100644
--- a/drivers/mtd/spi/sf_internal.h
+++ b/drivers/mtd/spi/sf_internal.h
@@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash);
 #if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
 int spi_flash_mtd_register(struct spi_flash *flash);
 void spi_flash_mtd_unregister(void);
+#else
+static inline int spi_flash_mtd_register(struct spi_flash *flash)
+{
+	return 0;
+}
+
+static inline void spi_flash_mtd_unregister(void)
+{
+}
 #endif
+
 #endif /* _SF_INTERNAL_H_ */
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
index 89e384901c..1e8744896c 100644
--- a/drivers/mtd/spi/sf_probe.c
+++ b/drivers/mtd/spi/sf_probe.c
@@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash *flash)
 	if (ret)
 		goto err_read_id;
 
-#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
-	ret = spi_flash_mtd_register(flash);
-#endif
+	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
+		ret = spi_flash_mtd_register(flash);
 
 err_read_id:
 	spi_release_bus(spi);
@@ -83,9 +82,9 @@ struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs,
 
 void spi_flash_free(struct spi_flash *flash)
 {
-#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
-	spi_flash_mtd_unregister();
-#endif
+	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
+		spi_flash_mtd_unregister();
+
 	spi_free_slave(flash->spi);
 	free(flash);
 }
@@ -150,9 +149,9 @@ int spi_flash_std_probe(struct udevice *dev)
 
 static int spi_flash_std_remove(struct udevice *dev)
 {
-#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
-	spi_flash_mtd_unregister();
-#endif
+	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
+		spi_flash_mtd_unregister();
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH 5/5] mtd: spi: Use IS_ENABLED to prevent ifdef

[Daniel Schwierzeck]

Am 14.05.20 um 14:11 schrieb Jagan Teki:
> Use IS_ENABLED to prevent ifdef in sf_probe.c
> 
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Vignesh R <vigneshr@ti.com>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/mtd/spi/sf_internal.h | 10 ++++++++++
>  drivers/mtd/spi/sf_probe.c    | 17 ++++++++---------
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
> index 940b2e4c9e..544ed74a5f 100644
> --- a/drivers/mtd/spi/sf_internal.h
> +++ b/drivers/mtd/spi/sf_internal.h
> @@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash);
>  #if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>  int spi_flash_mtd_register(struct spi_flash *flash);
>  void spi_flash_mtd_unregister(void);
> +#else
> +static inline int spi_flash_mtd_register(struct spi_flash *flash)
> +{
> +	return 0;
> +}
> +
> +static inline void spi_flash_mtd_unregister(void)
> +{
> +}
>  #endif
> +
>  #endif /* _SF_INTERNAL_H_ */
> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> index 89e384901c..1e8744896c 100644
> --- a/drivers/mtd/spi/sf_probe.c
> +++ b/drivers/mtd/spi/sf_probe.c
> @@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash *flash)
>  	if (ret)
>  		goto err_read_id;
>  
> -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
> -	ret = spi_flash_mtd_register(flash);
> -#endif
> +	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
> +		ret = spi_flash_mtd_register(flash);

you have to use CONFIG_IS_ENABLED() instead of IS_ENABLED() to also
consider the existing CONFIG_SPL_SPI_FLASH_MTD option

$ git grep -n SPI_FLASH_MTD -- drivers/mtd/spi/
drivers/mtd/spi/Kconfig:187:config SPI_FLASH_MTD
drivers/mtd/spi/Kconfig:199:config SPL_SPI_FLASH_MTD


>  
>  err_read_id:
>  	spi_release_bus(spi);
> @@ -83,9 +82,9 @@ struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs,
>  
>  void spi_flash_free(struct spi_flash *flash)
>  {
> -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
> -	spi_flash_mtd_unregister();
> -#endif
> +	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
> +		spi_flash_mtd_unregister();
> +
>  	spi_free_slave(flash->spi);
>  	free(flash);
>  }
> @@ -150,9 +149,9 @@ int spi_flash_std_probe(struct udevice *dev)
>  
>  static int spi_flash_std_remove(struct udevice *dev)
>  {
> -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
> -	spi_flash_mtd_unregister();
> -#endif
> +	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
> +		spi_flash_mtd_unregister();
> +
>  	return 0;
>  }
>  
> 

-- 
- Daniel

[PATCH 5/5] mtd: spi: Use IS_ENABLED to prevent ifdef

[Stefan Roese]

Hi Daniel,

On 14.05.20 18:31, Daniel Schwierzeck wrote:
> 
> 
> Am 14.05.20 um 14:11 schrieb Jagan Teki:
>> Use IS_ENABLED to prevent ifdef in sf_probe.c
>>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Vignesh R <vigneshr@ti.com>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>> ---
>>   drivers/mtd/spi/sf_internal.h | 10 ++++++++++
>>   drivers/mtd/spi/sf_probe.c    | 17 ++++++++---------
>>   2 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
>> index 940b2e4c9e..544ed74a5f 100644
>> --- a/drivers/mtd/spi/sf_internal.h
>> +++ b/drivers/mtd/spi/sf_internal.h
>> @@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash);
>>   #if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>>   int spi_flash_mtd_register(struct spi_flash *flash);
>>   void spi_flash_mtd_unregister(void);
>> +#else
>> +static inline int spi_flash_mtd_register(struct spi_flash *flash)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void spi_flash_mtd_unregister(void)
>> +{
>> +}
>>   #endif
>> +
>>   #endif /* _SF_INTERNAL_H_ */
>> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
>> index 89e384901c..1e8744896c 100644
>> --- a/drivers/mtd/spi/sf_probe.c
>> +++ b/drivers/mtd/spi/sf_probe.c
>> @@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash *flash)
>>   	if (ret)
>>   		goto err_read_id;
>>   
>> -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>> -	ret = spi_flash_mtd_register(flash);
>> -#endif
>> +	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
>> +		ret = spi_flash_mtd_register(flash);
> 
> you have to use CONFIG_IS_ENABLED() instead of IS_ENABLED()

Just curious: I thought that those two are equivalent:

IS_ENABLED(CONFIG_FOO) and
CONFIG_IS_ENABLED(FOO)

Is this not the case? From a functional point of view? I personally
prefer IS_ENABLED(CONFIG_FOO), as the Kconfig symbol is completely
visible this way.

Thanks,
Stefan

> to also
> consider the existing CONFIG_SPL_SPI_FLASH_MTD option
> 
> $ git grep -n SPI_FLASH_MTD -- drivers/mtd/spi/
> drivers/mtd/spi/Kconfig:187:config SPI_FLASH_MTD
> drivers/mtd/spi/Kconfig:199:config SPL_SPI_FLASH_MTD
> 
> 
>>   
>>   err_read_id:
>>   	spi_release_bus(spi);
>> @@ -83,9 +82,9 @@ struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs,
>>   
>>   void spi_flash_free(struct spi_flash *flash)
>>   {
>> -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>> -	spi_flash_mtd_unregister();
>> -#endif
>> +	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
>> +		spi_flash_mtd_unregister();
>> +
>>   	spi_free_slave(flash->spi);
>>   	free(flash);
>>   }
>> @@ -150,9 +149,9 @@ int spi_flash_std_probe(struct udevice *dev)
>>   
>>   static int spi_flash_std_remove(struct udevice *dev)
>>   {
>> -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>> -	spi_flash_mtd_unregister();
>> -#endif
>> +	if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
>> +		spi_flash_mtd_unregister();
>> +
>>   	return 0;
>>   }
>>   
>>
> 

[PATCH 5/5] mtd: spi: Use IS_ENABLED to prevent ifdef

[Rasmus Villemoes]

On 15/05/2020 06.27, Stefan Roese wrote:
> Hi Daniel,
> 
> On 14.05.20 18:31, Daniel Schwierzeck wrote:
>>
>>
>> Am 14.05.20 um 14:11 schrieb Jagan Teki:
>>> Use IS_ENABLED to prevent ifdef in sf_probe.c
>>>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Vignesh R <vigneshr@ti.com>
>>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>> ? drivers/mtd/spi/sf_internal.h | 10 ++++++++++
>>> ? drivers/mtd/spi/sf_probe.c??? | 17 ++++++++---------
>>> ? 2 files changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi/sf_internal.h
>>> b/drivers/mtd/spi/sf_internal.h
>>> index 940b2e4c9e..544ed74a5f 100644
>>> --- a/drivers/mtd/spi/sf_internal.h
>>> +++ b/drivers/mtd/spi/sf_internal.h
>>> @@ -81,5 +81,15 @@ int spi_flash_cmd_get_sw_write_prot(struct
>>> spi_flash *flash);
>>> ? #if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>>> ? int spi_flash_mtd_register(struct spi_flash *flash);
>>> ? void spi_flash_mtd_unregister(void);
>>> +#else
>>> +static inline int spi_flash_mtd_register(struct spi_flash *flash)
>>> +{
>>> +??? return 0;
>>> +}
>>> +
>>> +static inline void spi_flash_mtd_unregister(void)
>>> +{
>>> +}
>>> ? #endif
>>> +
>>> ? #endif /* _SF_INTERNAL_H_ */
>>> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
>>> index 89e384901c..1e8744896c 100644
>>> --- a/drivers/mtd/spi/sf_probe.c
>>> +++ b/drivers/mtd/spi/sf_probe.c
>>> @@ -44,9 +44,8 @@ static int spi_flash_probe_slave(struct spi_flash
>>> *flash)
>>> ????? if (ret)
>>> ????????? goto err_read_id;
>>> ? -#if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>>> -??? ret = spi_flash_mtd_register(flash);
>>> -#endif
>>> +??? if (IS_ENABLED(CONFIG_SPI_FLASH_MTD))
>>> +??????? ret = spi_flash_mtd_register(flash);
>>
>> you have to use CONFIG_IS_ENABLED() instead of IS_ENABLED()
> 
> Just curious: I thought that those two are equivalent:
> 
> IS_ENABLED(CONFIG_FOO) and
> CONFIG_IS_ENABLED(FOO)
> 
> Is this not the case?

No. The latter must be used for the symbols FOO that also exist in a
separate SPL_FOO variant, while the former must be used where the same
Kconfig symbol is supposed to cover both SPL and U-Boot proper.

The former "morally" expands to (morally, there's some some preprocessor
trickery to deal with the fact that defined() doesn't work outside
preprocessor conditionals)

  (defined(CONFIG_FOO) && CONFIG_FOO == 1) ? 1 : 0

If FOO is a proper boolean Kconfig symbol, CONFIG_FOO is either defined
as 1 or undefined. But the above also works for the legacy things set in
a board header, where CONFIG_FOO could be explicitly defined as 0 or 1.

The CONFIG_IS_ENABLED(FOO) expands to exactly the same thing when
building U-Boot proper. But for SPL, the expansion is instead (again,
morally)

  (defined(CONFIG_SPL_FOO) && CONFIG_SPL_FOO == 1) ? CONFIG_SPL_FOO : 0

That should make it obvious why one needs a variant that doesn't want
the CONFIG_ included in the argument; the CONFIG_IS_ENABLED macro needs
to do token-pasting with either CONFIG_SPL_ or just CONFIG_.

[Implementation-wise, the trick to make the above work both for macros
that are not defined and those that are defined with the value 1 is to
have helpers

#define FIRST_ARGUMENT_1 blargh,
#define second_arg(one, two, ...) two

macro, then with the appropriate amount of indirections to make sure
macros get expanded and tokens get concatenated in the right order, one does

  second_arg(EAT_AN_ARGUMENT_##${CONFIG_FOO} 1, 0)

If CONFIG_FOO is a macro with the value 1, the above becomes

  second_arg(FIRST_ARGUMENT_1 1, 0) ->
  second_arg(blarg, 1, 0) ->
  1

while if CONFIG_FOO is not defined, the token just "expands" to itself,
so we get

  second_arg(FIRST_ARGUMENT_CONFIG_FOO 1, 0)

where, since FIRST_ARGUMENT_CONFIG_FOO also doesn't expand further, we
get the 0. The same works if CONFIG_FOO is defined to any value other
than 1, as long as its expansion is something that is valid for token
concatenation; in particular, if it has the value 0, one just gets
second_arg(FIRST_ARGUMENT_0 1, 0) where again FIRST_ARGUMENT_0 doesn't
expand further and provide another comma, so the 0 gets picked out.]

  second_arg(blarg, 1, 0) ->
  1

]

[PATCH 5/5] mtd: spi: Use IS_ENABLED to prevent ifdef

[Rasmus Villemoes]

On 15/05/2020 09.22, Rasmus Villemoes wrote:

> That should make it obvious why one needs a variant that doesn't want
> the CONFIG_ included in the argument; the CONFIG_IS_ENABLED macro needs
> to do token-pasting with either CONFIG_SPL_ or just CONFIG_.

On that note, I think all hits from

  git grep 'CONFIG_IS_ENABLED(CONFIG_'

are bugs.

Another "script" that might be worth doing is finding instances of
CONFIG_IS_ENABLED(FOO) where SPL_FOO doesn't exist as a Kconfig symbol
(or perhaps in the whitelist), and conversely finding IS_ENABLED(FOO)
when there actually is a separate SPL_FOO symbol.

Rasmus