[U-Boot] [PATCH v3 1/5] mtd: Use default mtdparts/mtids when not defined in the environment

[Marek Vasut]

On 11/13/2018 03:37 PM, Boris Brezillon wrote:
> On Tue, 13 Nov 2018 15:10:14 +0100
> Marek Vasut <marek.vasut at 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 at 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 at denx.de>
>>>>> Signed-off-by: Boris Brezillon <boris.brezillon at bootlin.com>
>>>>> Tested-by: Stefan Roese <sr at denx.de>
>>>>> Reviewed-by: Lukasz Majewski <lukma at 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).

Fine by me , and yes, that can be put on ToDo.

-- 
Best regards,
Marek Vasut