On 2018-07-16 18:23, Boris Brezillon wrote: > Hi Rafal, > > On Mon, 16 Jul 2018 13:17:12 +0200 > Rafa? Mi?ecki <zajec5 at gmail.com> wrote: > >> From: Rafa? Mi?ecki <rafal at milecki.pl> >> >> Existing implementation of specifying flash device parsers became a >> bit >> hacky and needs a cleanup. Currently it requires: >> 1) Passing parsers in a custom platform data by arch code >> or >> 2) Hardcoding parsers table in a flash driver >> >> The purpose of the new implementation is to: >> 1) Clean up flash drivers >> 2) Have a generic solution >> 3) Avoid code duplication >> >> That new implementation assigns table of parsers to a MTD's parent >> device. That way flash driver doesn't have to take care of passing >> parsers. It's inspired by GPIO lookup table. >> >> Signed-off-by: Rafa? Mi?ecki <rafal at milecki.pl> >> --- >> It's obviously a RFC patch only. It doesn't really implement anything. >> It DOESN'T COMPILE. >> >> I'd like to know if you like this idea and if it's worth for me to >> actually spend time implementing it. > > I love the idea. Actually, I'd like to extend that to fixed partition > definitions (those passed to mtd_device_register()) so that they can be > parsed by the fixed-partition parser (the one already taking care of > compatible = "fixed-partitions"). That was something I planned to handle next, so it's great to hear we share the ideas for further improvements! That will extremely simplify mtd_device_register(). > One comment below. > >> --- >> arch/arm/mach-pxa/corgi.c | 21 +++++++++++++-------- >> drivers/mtd/mtdpart.c | 4 ++++ >> 2 files changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c >> index 9a5a35e90769..0ff77e078d2b 100644 >> --- a/arch/arm/mach-pxa/corgi.c >> +++ b/arch/arm/mach-pxa/corgi.c >> @@ -615,16 +615,8 @@ static struct nand_bbt_descr sharpsl_bbt = { >> .pattern = scan_ff_pattern >> }; >> >> -static const char * const probes[] = { >> - "cmdlinepart", >> - "ofpart", >> - "sharpslpart", >> - NULL, >> -}; >> - >> static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = >> { >> .badblock_pattern = &sharpsl_bbt, >> - .part_parsers = probes, >> }; >> >> static struct resource sharpsl_nand_resources[] = { >> @@ -688,6 +680,16 @@ static struct i2c_board_info __initdata >> corgi_i2c_devices[] = { >> { I2C_BOARD_INFO("wm8731", 0x1b) }, >> }; >> >> +static struct mtd_parser_lookup_table corgi_mtd_parser_lookup_table = >> { >> + .dev_name = NULL, /* Set with registered name */ > > Can't you guess the device name? It's really weird that you have to > register the lookup table after the device has been registered. I > already had a similar discussion on GPIO lookup tables registration on > ams-delta here [1], and I think my comments apply to this case as well. > > (...) > > [1]https://patchwork.ozlabs.org/patch/920798/ Thanks for link to that discussion! I saw a possible race problem regarding registering lookup table and probing a platform (?) device/driver. I thought we can just depend on arch code being executed very early. Your solution with guessing device name should work in 99% of cases. If you ls /sys/devices/platform/, you'll see that names are assigned using name.<index> pattern. In 99% cases there is only one place in the code registering a given platform device. As there is only one place registering "sharpsl-nand" device, we can assume the resulting name will be "sharpsl-nand.0". If there was another code doing the same, you couldn't know if you should expect "sharpsl-nand.0" or "sharpsl-nand.1". I believe we should be fine guessing in these 99% cases and only depend on order (semi-risking a race) in that 1% (or less).