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@xxxxxxxxx>
> > Cc: Tony Lindgren <tony@xxxxxxxxxxx>
> > Cc: Aaro Koskinen <aaro.koskinen@xxxxxx>
> > ---
> > 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;
> >
>
