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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/