Re: [PATCH V2] mtd: call of_platform_populate() for MTD partitions

Rafał Miłecki
2022-05-10T05:56:32Z

On 9.05.2022 16:17, Miquel Raynal wrote:
> zajec5@gmail.com wrote on Wed, 4 May 2022 21:44:48 +0200:
>
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Until this change MTD subsystem supported handling partitions only with
>> MTD partitions parsers. That's a specific / limited API designed around
>> partitions.
>>
>> Some MTD partitions may however require different handling. They may
>> contain specific data that needs to be parsed and somehow extracted. For
>> that purpose MTD subsystem should allow binding of standard platform
>> drivers.
>>
>> An example can be U-Boot (sub)partition with environment variables.
>> There exist a "u-boot,env" DT binding for MTD (sub)partition that
>> requires an NVMEM driver.
>>
>> Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> V2: Fix case for # CONFIG_MTD_PARTITIONED_MASTER is not set
>> master->dev can't be used blindly as it may point to unregistered
>> device and cause WARNINGs
>> ---
>> drivers/mtd/mtdpart.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>> index 357661b62c94..4971fa69d076 100644
>> --- a/drivers/mtd/mtdpart.c
>> +++ b/drivers/mtd/mtdpart.c
>> @@ -17,6 +17,7 @@
>> #include <linux/mtd/partitions.h>
>> #include <linux/err.h>
>> #include <linux/of.h>
>> +#include <linux/of_platform.h>
>>
>> #include "mtdcore.h"
>>
>> @@ -577,10 +578,16 @@ static int mtd_part_of_parse(struct mtd_info *master,
>> struct mtd_part_parser *parser;
>> struct device_node *np;
>> struct property *prop;
>> + struct device *dev;
>> const char *compat;
>> const char *fixed = "fixed-partitions";
>> int ret, err = 0;
>>
>> + if (mtd_is_partition(master) || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
>
> Are you sure about this condition? Isn't accessing master->dev.parent
> going to fail if !IS_ENABLED(PARTITIONED_MASTER) ?
>
> I'm not 100% sure my remark is correct but I fail to get the logic
> here.

It seems to work as expected, I tested it using device with following
layout:

2 fixed-partitions partitions found on MTD device brcmnand.0
Creating 2 MTD partitions on "brcmnand.0":
0x000000000000-0x000000100000 : "loader"
1 fixed-partitions partitions found on MTD device loader
Creating 1 MTD partitions on "loader":
0x000000040000-0x000000044008 : "u-boot-env"
0x000000100000-0x00001ff00000 : "image"

brcmnand.0
├── loader
│ └── u-boot-env
└── image

# CONFIG_MTD_PARTITIONED_MASTER is not set

┌────────┬────────────┬────────────────────────────────────────┐
│ Device │ Name │ struct device *dev │
├────────┼────────────┼────────────────────────────────────────┤
│ - │ brcmnand.0 │ bcm63138_nand ff801800.nand-controller │
│ mtd0 │ loader │ mtd mtd0 │
│ mtd1 │ u-boot-env │ mtd mtd1 │
│ mtd2 │ image │ mtd mtd2 │
└────────┴────────────┴────────────────────────────────────────┘

CONFIG_MTD_PARTITIONED_MASTER=y

┌────────┬────────────┬────────────────────┐
│ Device │ Name │ struct device *dev │
├────────┼────────────┼────────────────────┤
│ mtd0 │ brcmnand.0 │ mtd0 │
│ mtd1 │ loader │ mtd mtd1 │
│ mtd2 │ u-boot-env │ mtd mtd2 │
│ mtd3 │ image │ mtd mtd3 │
└────────┴────────────┴────────────────────┘

As you can see the only tricky case is *master* mtd *without*
CONFIG_MTD_PARTITIONED_MASTER. In that case we don't register its device
and so a parent (NAND controller) has to be used instead. My condition
seems to handle that correctly.