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

Miquel Raynal
2022-05-10T08:13:17Z

Hi Rafał,

zajec5@gmail.com wrote on Tue, 10 May 2022 07:56:02 +0200:

> 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.

I think I get it now. I know it's a lot of not- conditions but I feel
the logic could be simpler to understand this way:

dev = mtd->dev;
/* Use the top device structure (the controller) if the top level MTD
parent is not registered */
if (!IS_ENABLED(PARTITIONED_MASTER) && !mtd_is_partition(mtd))

What do you think?

Thanks,
Miquèl