On 15/05/20 01:14PM, Jagan Teki wrote:
> On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.ya...@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 <s...@chromium.org>
> > > Cc: Vignesh R <vigne...@ti.com>
> > > Signed-off-by: Jagan Teki <ja...@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
