| Message ID | 20190424180212.10830-1-jmkrzyszt@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | [v3] mtd: rawnand: ams-delta: Drop board specific partition info | expand |
Hi Janusz, On Wed, Apr 24, 2019 at 08:02:12PM +0200, Janusz Krzysztofik wrote: > After recent modifications, only a hardcoded partition info makes > the driver device specific. Other than that, the driver uses GPIO > exclusively and can be used on any hardware. > > Drop the partition info and use MTD partition parser with default list > of parser names instead. For the OF parser to work correctly, pass > device of_node to mtd. > > Amstrad Delta users should append the following partition info to their > kernel command line, possibly embedding it in CONFIG_CMDLINE: > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved) now, when driver is no longer Amstrad Delta specific, why would you want to have ams-delta-nand hardcoded on kernel cmdline? I'm assuming at some point this driver will become gpio-nand [*] or something like that and asking users to change their kernel cmdline twice is just unwise :) [*] btw, it is really shame gpio-nand name is already taken by driver living in drivers/mtd/nand/raw/gpio.c which is more likely gpio-mem-nand used by at least CompuLab CM-X255 and Picochip picoXcell. Otherwise your work on this driver is so amazing that I just spent couple of hours finding that phone and compiling some decent userspace for it :) Thank you! > For their convenience, that information is added to the board Kconfig > entry help text, as well as CONFIG_MTD_CMDLINE_PARTS symbol is selected > automatically from the board Kconfig entry if the NAND driver is also > selected. > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > Cc: Tony Lindgren <tony@atomide.com> > Cc: Aaro Koskinen <aaro.koskinen@iki.fi> > --- > Changelog: > v2->v3: > - add information on the requirement for passing partition info via > kernel command line to the board Kconfig entry help text > v1->v2: > - fix a typo poitned out by Aaro - thanks! > - fix device_node not passed to OF parser via mtd_info > - commit message reworded and reformatted a bit for better readability > > arch/arm/mach-omap1/Kconfig | 7 ++++++- > drivers/mtd/nand/raw/ams-delta.c | 29 ++--------------------------- > 2 files changed, 8 insertions(+), 28 deletions(-) > > diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig > index c4694f26b5c4..41a47d251cac 100644 > --- a/arch/arm/mach-omap1/Kconfig > +++ b/arch/arm/mach-omap1/Kconfig > @@ -164,17 +164,22 @@ config MACH_NOKIA770 > have such a device. > > config MACH_AMS_DELTA > - bool "Amstrad E3 (Delta)" > + bool "Amstrad E3 (Delta) - see help for important information" > depends on ARCH_OMAP1 && ARCH_OMAP15XX > select FIQ > select GPIO_GENERIC_PLATFORM > select LEDS_GPIO_REGISTER > select REGULATOR > select REGULATOR_FIXED_VOLTAGE > + select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA > help > Support for the Amstrad E3 (codename Delta) videophone. Say Y here > if you have such a device. > > + If you are using built-in NAND, append the following partition > + info to kernel command line: > + mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved) > + > config MACH_OMAP_GENERIC > bool "Generic OMAP board" > depends on ARCH_OMAP1 && (ARCH_OMAP15XX || ARCH_OMAP16XX) > diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c > index 8312182088c1..e0f09179bbda 100644 > --- a/drivers/mtd/nand/raw/ams-delta.c > +++ b/drivers/mtd/nand/raw/ams-delta.c > @@ -41,31 +41,6 @@ struct ams_delta_nand { > bool data_in; > }; > > -/* > - * Define partitions for flash devices > - */ > - > -static const struct mtd_partition partition_info[] = { > - { .name = "Kernel", > - .offset = 0, > - .size = 3 * SZ_1M + SZ_512K }, > - { .name = "u-boot", > - .offset = 3 * SZ_1M + SZ_512K, > - .size = SZ_256K }, > - { .name = "u-boot params", > - .offset = 3 * SZ_1M + SZ_512K + SZ_256K, > - .size = SZ_256K }, > - { .name = "Amstrad LDR", > - .offset = 4 * SZ_1M, > - .size = SZ_256K }, > - { .name = "File system", > - .offset = 4 * SZ_1M + 1 * SZ_256K, > - .size = 27 * SZ_1M }, > - { .name = "PBL reserved", > - .offset = 32 * SZ_1M - 3 * SZ_256K, > - .size = 3 * SZ_256K }, > -}; > - > static void ams_delta_write_commit(struct ams_delta_nand *priv) > { > gpiod_set_value(priv->gpiod_nwe, 0); > @@ -238,6 +213,7 @@ static int ams_delta_init(struct platform_device *pdev) > mtd->dev.parent = &pdev->dev; > > nand_set_controller_data(this, priv); > + nand_set_flash_node(this, pdev->dev.of_node); > > priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > if (IS_ERR(priv->gpiod_rdy)) { > @@ -315,8 +291,7 @@ static int ams_delta_init(struct platform_device *pdev) > return err; > > /* Register the partitions */ > - err = mtd_device_register(mtd, partition_info, > - ARRAY_SIZE(partition_info)); > + err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0); > if (err) > goto err_nand_cleanup; > > -- > 2.21.0
Hi Ladislav, On Thursday, April 25, 2019 12:14:28 AM CEST Ladislav Michl wrote: > Hi Janusz, > > On Wed, Apr 24, 2019 at 08:02:12PM +0200, Janusz Krzysztofik wrote: > > After recent modifications, only a hardcoded partition info makes > > the driver device specific. Other than that, the driver uses GPIO > > exclusively and can be used on any hardware. > > > > Drop the partition info and use MTD partition parser with default list > > of parser names instead. For the OF parser to work correctly, pass > > device of_node to mtd. > > > > Amstrad Delta users should append the following partition info to their > > kernel command line, possibly embedding it in CONFIG_CMDLINE: > > > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved) > > now, when driver is no longer Amstrad Delta specific, why would you want > to have ams-delta-nand hardcoded on kernel cmdline? I'm assuming at some > point this driver will become gpio-nand [*] or something like that and > asking users to change their kernel cmdline twice is just unwise :) Hmm, I have no idea of a good name for the driver if not "gpio-nand". Can you suggest one? As a workaround, I can add a platform device id table to the driver with "ams- delta-nand" as a supported device name in hope that survives possible future driver renaming. > [*] btw, it is really shame gpio-nand name is already taken by driver > living in drivers/mtd/nand/raw/gpio.c which is more likely gpio-mem-nand > used by at least CompuLab CM-X255 and Picochip picoXcell. I think the best approach would be to expose NAND data ports of those machines as GPIO ports, possibly reusing the "gpio-nand" driver code while creating a new GPIO driver for them if "basic-mmio-gpio" occurs inappropriate, and use the pure GPIO NAND driver on top. > Otherwise your work on this driver is so amazing that I just spent > couple of hours finding that phone and compiling some decent userspace > for it :) Thank you! I'm glad you like it :-) Janusz > > > For their convenience, that information is added to the board Kconfig > > entry help text, as well as CONFIG_MTD_CMDLINE_PARTS symbol is selected > > automatically from the board Kconfig entry if the NAND driver is also > > selected. > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > Cc: Tony Lindgren <tony@atomide.com> > > Cc: Aaro Koskinen <aaro.koskinen@iki.fi> > > --- > > Changelog: > > v2->v3: > > - add information on the requirement for passing partition info via > > kernel command line to the board Kconfig entry help text > > v1->v2: > > - fix a typo poitned out by Aaro - thanks! > > - fix device_node not passed to OF parser via mtd_info > > - commit message reworded and reformatted a bit for better readability > > > > arch/arm/mach-omap1/Kconfig | 7 ++++++- > > drivers/mtd/nand/raw/ams-delta.c | 29 ++--------------------------- > > 2 files changed, 8 insertions(+), 28 deletions(-) > > > > diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig > > index c4694f26b5c4..41a47d251cac 100644 > > --- a/arch/arm/mach-omap1/Kconfig > > +++ b/arch/arm/mach-omap1/Kconfig > > @@ -164,17 +164,22 @@ config MACH_NOKIA770 > > have such a device. > > > > config MACH_AMS_DELTA > > - bool "Amstrad E3 (Delta)" > > + bool "Amstrad E3 (Delta) - see help for important information" > > depends on ARCH_OMAP1 && ARCH_OMAP15XX > > select FIQ > > select GPIO_GENERIC_PLATFORM > > select LEDS_GPIO_REGISTER > > select REGULATOR > > select REGULATOR_FIXED_VOLTAGE > > + select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA > > help > > Support for the Amstrad E3 (codename Delta) videophone. Say Y here > > if you have such a device. > > > > + If you are using built-in NAND, append the following partition > > + info to kernel command line: > > + mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u- boot_params),256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved) > > + > > config MACH_OMAP_GENERIC > > bool "Generic OMAP board" > > depends on ARCH_OMAP1 && (ARCH_OMAP15XX || ARCH_OMAP16XX) > > diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams- delta.c > > index 8312182088c1..e0f09179bbda 100644 > > --- a/drivers/mtd/nand/raw/ams-delta.c > > +++ b/drivers/mtd/nand/raw/ams-delta.c > > @@ -41,31 +41,6 @@ struct ams_delta_nand { > > bool data_in; > > }; > > > > -/* > > - * Define partitions for flash devices > > - */ > > - > > -static const struct mtd_partition partition_info[] = { > > - { .name = "Kernel", > > - .offset = 0, > > - .size = 3 * SZ_1M + SZ_512K }, > > - { .name = "u-boot", > > - .offset = 3 * SZ_1M + SZ_512K, > > - .size = SZ_256K }, > > - { .name = "u-boot params", > > - .offset = 3 * SZ_1M + SZ_512K + SZ_256K, > > - .size = SZ_256K }, > > - { .name = "Amstrad LDR", > > - .offset = 4 * SZ_1M, > > - .size = SZ_256K }, > > - { .name = "File system", > > - .offset = 4 * SZ_1M + 1 * SZ_256K, > > - .size = 27 * SZ_1M }, > > - { .name = "PBL reserved", > > - .offset = 32 * SZ_1M - 3 * SZ_256K, > > - .size = 3 * SZ_256K }, > > -}; > > - > > static void ams_delta_write_commit(struct ams_delta_nand *priv) > > { > > gpiod_set_value(priv->gpiod_nwe, 0); > > @@ -238,6 +213,7 @@ static int ams_delta_init(struct platform_device *pdev) > > mtd->dev.parent = &pdev->dev; > > > > nand_set_controller_data(this, priv); > > + nand_set_flash_node(this, pdev->dev.of_node); > > > > priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > if (IS_ERR(priv->gpiod_rdy)) { > > @@ -315,8 +291,7 @@ static int ams_delta_init(struct platform_device *pdev) > > return err; > > > > /* Register the partitions */ > > - err = mtd_device_register(mtd, partition_info, > > - ARRAY_SIZE(partition_info)); > > + err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0); > > if (err) > > goto err_nand_cleanup; > > >
On Thu, Apr 25, 2019 at 08:42:22PM +0200, Janusz Krzysztofik wrote: > Hi Ladislav, > > On Thursday, April 25, 2019 12:14:28 AM CEST Ladislav Michl wrote: > > Hi Janusz, > > > > On Wed, Apr 24, 2019 at 08:02:12PM +0200, Janusz Krzysztofik wrote: > > > After recent modifications, only a hardcoded partition info makes > > > the driver device specific. Other than that, the driver uses GPIO > > > exclusively and can be used on any hardware. > > > > > > Drop the partition info and use MTD partition parser with default list > > > of parser names instead. For the OF parser to work correctly, pass > > > device of_node to mtd. > > > > > > Amstrad Delta users should append the following partition info to their > > > kernel command line, possibly embedding it in CONFIG_CMDLINE: > > > > > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved) > > > > now, when driver is no longer Amstrad Delta specific, why would you want > > to have ams-delta-nand hardcoded on kernel cmdline? I'm assuming at some > > point this driver will become gpio-nand [*] or something like that and > > asking users to change their kernel cmdline twice is just unwise :) > > Hmm, I have no idea of a good name for the driver if not "gpio-nand". Can you > suggest one? gpio-nand is so good name that it should be worth merging gpio.c and ams-delta.c into gpio_nand.c :) > As a workaround, I can add a platform device id table to the driver with "ams- > delta-nand" as a supported device name in hope that survives possible future > driver renaming. > > > [*] btw, it is really shame gpio-nand name is already taken by driver > > living in drivers/mtd/nand/raw/gpio.c which is more likely gpio-mem-nand > > used by at least CompuLab CM-X255 and Picochip picoXcell. > > I think the best approach would be to expose NAND data ports of those machines > as GPIO ports, possibly reusing the "gpio-nand" driver code while creating a > new GPIO driver for them if "basic-mmio-gpio" occurs inappropriate, and use > the pure GPIO NAND driver on top. What about adding two fields into struct ams_delta_nand holding pointers to either ams_delta_{read,write}_buf (renamed to gpio_nand_...) or mmio r/w functions depending on driver configuration? > > Otherwise your work on this driver is so amazing that I just spent > > couple of hours finding that phone and compiling some decent userspace > > for it :) Thank you! > > I'm glad you like it :-) > Janusz Best regards, ladis
diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig index c4694f26b5c4..41a47d251cac 100644 --- a/arch/arm/mach-omap1/Kconfig +++ b/arch/arm/mach-omap1/Kconfig @@ -164,17 +164,22 @@ config MACH_NOKIA770 have such a device. config MACH_AMS_DELTA - bool "Amstrad E3 (Delta)" + bool "Amstrad E3 (Delta) - see help for important information" depends on ARCH_OMAP1 && ARCH_OMAP15XX select FIQ select GPIO_GENERIC_PLATFORM select LEDS_GPIO_REGISTER select REGULATOR select REGULATOR_FIXED_VOLTAGE + select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA help Support for the Amstrad E3 (codename Delta) videophone. Say Y here if you have such a device. + If you are using built-in NAND, append the following partition + info to kernel command line: + mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved) + config MACH_OMAP_GENERIC bool "Generic OMAP board" depends on ARCH_OMAP1 && (ARCH_OMAP15XX || ARCH_OMAP16XX) diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c index 8312182088c1..e0f09179bbda 100644 --- a/drivers/mtd/nand/raw/ams-delta.c +++ b/drivers/mtd/nand/raw/ams-delta.c @@ -41,31 +41,6 @@ struct ams_delta_nand { bool data_in; }; -/* - * Define partitions for flash devices - */ - -static const struct mtd_partition partition_info[] = { - { .name = "Kernel", - .offset = 0, - .size = 3 * SZ_1M + SZ_512K }, - { .name = "u-boot", - .offset = 3 * SZ_1M + SZ_512K, - .size = SZ_256K }, - { .name = "u-boot params", - .offset = 3 * SZ_1M + SZ_512K + SZ_256K, - .size = SZ_256K }, - { .name = "Amstrad LDR", - .offset = 4 * SZ_1M, - .size = SZ_256K }, - { .name = "File system", - .offset = 4 * SZ_1M + 1 * SZ_256K, - .size = 27 * SZ_1M }, - { .name = "PBL reserved", - .offset = 32 * SZ_1M - 3 * SZ_256K, - .size = 3 * SZ_256K }, -}; - static void ams_delta_write_commit(struct ams_delta_nand *priv) { gpiod_set_value(priv->gpiod_nwe, 0); @@ -238,6 +213,7 @@ static int ams_delta_init(struct platform_device *pdev) mtd->dev.parent = &pdev->dev; nand_set_controller_data(this, priv); + nand_set_flash_node(this, pdev->dev.of_node); priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); if (IS_ERR(priv->gpiod_rdy)) { @@ -315,8 +291,7 @@ static int ams_delta_init(struct platform_device *pdev) return err; /* Register the partitions */ - err = mtd_device_register(mtd, partition_info, - ARRAY_SIZE(partition_info)); + err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0); if (err) goto err_nand_cleanup;
After recent modifications, only a hardcoded partition info makes the driver device specific. Other than that, the driver uses GPIO exclusively and can be used on any hardware. Drop the partition info and use MTD partition parser with default list of parser names instead. For the OF parser to work correctly, pass device of_node to mtd. Amstrad Delta users should append the following partition info to their kernel command line, possibly embedding it in CONFIG_CMDLINE: mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved) For their convenience, that information is added to the board Kconfig entry help text, as well as CONFIG_MTD_CMDLINE_PARTS symbol is selected automatically from the board Kconfig entry if the NAND driver is also selected. Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> Cc: Tony Lindgren <tony@atomide.com> Cc: Aaro Koskinen <aaro.koskinen@iki.fi> --- Changelog: v2->v3: - add information on the requirement for passing partition info via kernel command line to the board Kconfig entry help text v1->v2: - fix a typo poitned out by Aaro - thanks! - fix device_node not passed to OF parser via mtd_info - commit message reworded and reformatted a bit for better readability arch/arm/mach-omap1/Kconfig | 7 ++++++- drivers/mtd/nand/raw/ams-delta.c | 29 ++--------------------------- 2 files changed, 8 insertions(+), 28 deletions(-)