On Thu, Oct 17, 2013 at 04:42:23AM +0000, Pekon Gupta wrote: > > From: Brian Norris [mailto:computersforpeace@xxxxxxxxx] > > > On Tue, Oct 15, 2013 at 11:19:52AM +0530, Pekon Gupta wrote: > > > Autodetection of NAND device bus-width was added in generic NAND > > driver as > [...] > > > @@ -1904,6 +1903,21 @@ static int omap_nand_probe(struct > > platform_device *pdev) > > > nand_chip->chip_delay = 50; > > > } > > > > > > + /* scan for NAND device connected to chip controller */ > > > + if (nand_scan_ident(mtd, 1, NULL)) { > > > + err = -ENXIO; > > > + goto out_release_mem_region; > > > + } > > > + if ((nand_chip->options & NAND_BUSWIDTH_16) != > > > + (pdata->devsize & NAND_BUSWIDTH_16)) { > > > + pr_err("%s: detected %s device but driver configured for > > %s\n", > > > + DRIVER_NAME, > > > + (nand_chip->options & NAND_BUSWIDTH_16) ? > > "x16" : "x8", > > > + (pdata->devsize & NAND_BUSWIDTH_16) ? "x16" : > > "x8"); > > > > I'm not sure on the policy choice here; if you're using BUSWIDTH_AUTO, > > do you even want to try to configure the buswidth by platform data? > > You're not really getting the full use out of NAND_BUSWIDTH_AUTO because > > the platform data has to guess the same buswidth that nand_base.c > > determines. This is worse than the previous behavior, in which you would > > try both buswidths if the specified type failed. > > > > IOW, what are you trying to achieve with auto-buswidth? Automatic > > determination of the buswidth or just automatic validation? What do you > > achieve with platform data's selection of buswidth? Specification of the > > buswidth or just a hint? Do the goals conflict? Perhaps you can just > > warn, and not error out if the two selections don't match? Or maybe you > > really wanted to replace the platform data selection mechanism entirely > > with auto-buswidth? > > > This is 'automatic validation' of value set in DT binding "nand-bus-width" You mean you intend for the patched driver to simply validate, not configure the buswidth? > So this approach is other way round, where controller is configured > based on DT binding "nand-bus-width" and then it checks whether > device actual buswidth matches DT binding value or not. If this is the case, then you really don't want to use NAND_BUSWIDTH_AUTO. The default nand_base.c just *validates* the pre-selected buswidth and exits with an error, in much the same way that you're doing here (you're just duplicating the functionality). NAND_BUSWIDTH_AUTO is useful if you want to turn control of the buswidth configuration entirely over to nand_base.c. > - GPMC controller is pre-configured to support "x8" or "x16" device based > on DT binding "nand-bus-width". > Reference: $LINUX/arch/arm/mach-omap2/gpmc.c > @@gpmc_probe_nand_child() > val = of_get_nand_bus_width(child); > > - But, bus-width of NAND device is only known during NAND driver > Probe in nand_scan_ident(). > > Going forward when all NAND devices are compliant to ONFI, > then we can deprecate "nand-bus-width". Buswidth auto-detection is not actually restricted to ONFI. nand_base.c has (corretly, AFAIK) been detecting buswidths for a long time, using some form of ID decode detection. But it was just that: detection. It did not actually configure the buswidth, since it left that responsibility to the low-level driver to get right. Another thing to consider: I think this current patch is a regression from previous behavior. Previously, you would run nand_scan_ident() twice if the first one failed; once with the platform-configured buswidth and once with the opposite. But now, you only run nand_scan_ident() once, and then you quit if it detects differently than you expected. My opinion: the platform- and device-tree-provided buswidth is unnecessary; it was previouisly only a suggestion which your driver would readily discard, and it isn't really needed now. You can probably get by with just NAND_BUSWIDTH_AUTO and (at most) a warning if the auto-configured buswidth is different than the platform specified. But the real point: you need to clearly communicate what you are choosing in this patch. Either you want to (1) strictly follow the buswidth provided by the platform or (2) use the nand_base.c BUSWIDTH_AUTO automatic configuration. Trying to mix both (as your patch currently does) just makes everything worse. Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
