From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Brezillon <boris.brezillon@bootlin.com>
Date: Tue, 13 Nov 2018 15:37:26 +0100
Subject: [U-Boot] [PATCH v3 1/5] mtd: Use default mtdparts/mtids when
 not defined in the environment
In-Reply-To: <1272596c-a789-7a93-0198-3aacdd169194@gmail.com>
References: <20181113114313.30387-1-boris.brezillon@bootlin.com>
 <fd117ee9-b82b-f5a0-4704-f6916da9822a@gmail.com>
 <20181113133951.12ffd7b6@bbrezillon>
 <1272596c-a789-7a93-0198-3aacdd169194@gmail.com>
Message-ID: <20181113153726.5a586d05@bbrezillon>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Tue, 13 Nov 2018 15:10:14 +0100
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 11/13/2018 01:39 PM, Boris Brezillon wrote:
> > Hi Marek,
> > 
> > On Tue, 13 Nov 2018 13:19:52 +0100
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >   
> >> On 11/13/2018 12:43 PM, Boris Brezillon wrote:  
> >>> U-boot provides a mean to define default values for mtdids and mtdparts
> >>> when they're not defined in the environment. Patch mtd_probe_devices()
> >>> to use those default values when env_get("mtdparts") or
> >>> env_get("mtdids") return NULL.
> >>>
> >>> This implementation is based on the logic found in cmd/mtdparts.c.
> >>>
> >>> Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command")
> >>> Reported-by: Stefan Roese <sr@denx.de>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>> Tested-by: Stefan Roese <sr@denx.de>
> >>> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> >>> ---
> >>> Changes in v3:
> >>> - Fix env_get_f() call
> >>> - Add Lukasz R-b
> >>>
> >>> Changes in v2:
> >>> - none
> >>> ---
> >>>  drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++--
> >>>  1 file changed, 60 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
> >>> index 7d7a11c990d6..5ca560c96879 100644
> >>> --- a/drivers/mtd/mtd_uboot.c
> >>> +++ b/drivers/mtd/mtd_uboot.c
> >>> @@ -92,12 +92,70 @@ static void mtd_probe_uclass_mtd_devs(void) { }
> >>>  #endif
> >>>  
> >>>  #if defined(CONFIG_MTD_PARTITIONS)
> >>> +extern void board_mtdparts_default(const char **mtdids,
> >>> +				   const char **mtdparts);    
> >>
> >> Why is there this extern ?  
> > 
> > extern is not needed indeed.
> >   
> >> This should use a prototype of function
> >> defined in a header file. Once someone changes the prototype and forgets
> >> to update this location, it'll be a disaster.  
> > 
> > When I provide a fix I try to avoid dependencies on changes that are
> > not absolutely required hence the decision to keep this function
> > prototype locally defined.  
> 
> I am also expecting a patch which cleans this up then.
> I think __weak function would be appropriate.

Well, I think the best would be to get rid of this function. It's only
implemented by board/isee/igep00x0/igep00x0.c, and we might be able to
use CONFIG_{MTDPARTS,MTDIDS}_DEFAULT to replace it:

CONFIG_MTDPARTS_DEFAULT "omap2-nand:Xk(SPL),-(UBI);omap2-onenand:Yk(SPL),-(UBI)"
CONFIG_MTDIDS_DEFAULT "nand0=omap2-nand,onenand0=omap2-onenand"

This assumes we know the eraseblock size of the NAND and OneNAND devices of
course (X and Y in the above def).