On Fri, Aug 14, 2015 at 05:32:34AM +0200, Marek Vasut wrote: > On Friday, August 14, 2015 at 05:28:12 AM, Marek Vasut wrote: > > > + /* Get flash device data */ > > + for_each_available_child_of_node(dev->of_node, np) { ... > --->8--- > > > + /* > > + * Here is a 'nasty hack' from Marek Vasut to pick > > + * up OF properties from flash device subnode. > > + */ > > + nor->dev->of_node = np; > > + > > + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); > > + if (ret) > > + goto probe_failed; > > The only bizzare thing is this stuff above ^ . If I want to pass for example > "m25p,fast-read" to the SPI NOR connected to this controller, I have to set Do we really want to extend m25p80 properties like 'm25p,fast-read' to all SPI NOR controllers? Are they necessary? It seems that there has been some attempt to do so, but I'm not sure if that's by good design or just by accident. > up the nor->dev->of_node, otherwise the of_node would point to the controller. > I am positive this is wrong, but I'm not quite sure how to repair this. > Brian, can you please comment on this one bit ? The problem is that spi_nor_scan() is assuming that nor->dev represents a flash device, not a flash controller (to which we might connect multiple flash). So we need to provide a way for spi_nor_scan() to find the flash device_node, not the controller device_node. Easy option: (a) add a field to struct spi_nor, like I did to nand_chip [1]; e.g., spi_nor::dn. In doing that, we might then reevaluate what spi_nor::dev is supposed to mean (and clarify the doc comments in include/linux/mtd/spi-nor.h accordingly). Currently, it's used to shoe-horn in DT support (badly), as well as to provide mostly auxiliary info, like naming, debug info (dev_{dbg,info,err,etc...}()), and simliar. The latter half can actually be problematic too, since they start to become less useful once you have more than one flash connected to a single controller. e.g., you'll get colliding MTD names (via dev_name(nor->dev)) and debug info is suddenly less obvious (which flash chip is the log message from?). So, we might want to do something in the long run to avoid the mixing of layers that looks more like: (b) remove 'dev' from struct spi_nor entirely We can do debug prints and such with spi-nor-specific printk() formatters (e.g., snor_{info,dbg,err,etc...}()) and stop assuming that dev_name(nor->dev) is actually a good name for an MTD. While we're at it... we may also want a larger rework to allow spi-nor.c to better support a notion of controllers (using the existing platform device) and flash devices (mostly without the use of struct device, and mostly using struct spi_nor as-is). You'll notice that controllers that want to support multiple flash are starting to develop much of the same initialization boiler-plate code. Anyway, that's my rambling thoughts for now. I think (a) is pretty straightforward, correct, and quickly attainable, so you can ignore the remaining bits in the context of upstreaming this driver. Brian [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5844feeaa4154d1c46d3462c7a4653d22356d8b4 -- 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