On Thu, Oct 8, 2009 at 4:20 PM, Anton Vorontsov <avorontsov@xxxxxxxxxxxxx> wrote: > On Thu, Oct 08, 2009 at 09:48:50AM -0600, Grant Likely wrote: >> But the focus is still on creating pdata. If a translator gets too >> big, then sure, split it into a separate file. Until then, there I >> see no good reason to do so now. > > Luckily, I'm not at24 driver maintainer (Wolfram himself is ;-), > but as a maintainer of driver "Foo", I would not want to see > completely unfamiliar "Bar" in my shiny driver. It sounds like your saying that data parsing isn't really the same as driver code, and I don't think that is true. Device data parsing is equally as important as the functional behaviour. A device driver isn't complete unless it does both. Right now most drivers only understand LInux's internal representation (pdata) because that is the only data source they've needed to this point. When new data sources appear (device tree), it is completely appropriate for the driver to be modified to understand the new data format (with all the caveats of coding it in a logical way with translation decoupled from function to keep impact at a bare minimum). To use your example, a driver author who states "I only use Bar; so I don't ever want to see Foo code" is probably being a bit short sighted with regards to portability. > Another plus is that you can bypass (or almost bypass) subsystem > maintainers when merging OF-specific patches (since he/she couldn't > possibly care less about all these weird arch internals. But again, > this doesn't work for this particular driver since Wolfram is the > maintainer :-). I don't want OF parsing to bypass subsystem or driver maintainers. :-) I think they should be involved in reviewing and acking translator code. >> > If I wasn't a PPC/OF guy to some degree, I'd hate PPC/OF people >> > for bringing arch-specific details into a generic code... :-P >> >> No, this goes beyond PPC/OF. The real issue is that it is no longer a >> safe assumption that pdata will be a static data structure in platform >> code. The number of possible data sources is going to get larger, not >> smaller. OF is just one. UEFI is another. Translating that data >> into pdata will be the problem that comes up over and over again. >> However, translation code is still driver specific, >> so it belongs with the driver that it translates code for. > > Wait... The translation code depends on a platform, and on a > platform_data structure, the same as non-OF arch-specific code > depends on it. The translation code depends on the data source. That may be OF. It may be UEFI. It may be another driver (think a PCI driver instantiating a set of child platform devices). It may be a kernel hacker (who hard codes it into the platform code). It has nothing to do with arch/. (and ignore the whole of_platform bus stuff; that was a bad idea from the outset (not that we knew that at the time). I don't intend to port of_platform to other architectures). > How is it different from a static platform data > in the arch/ code? We don't put static platform data into the > drivers and surround them with ugly #ifdefs+machine_is()... Of course we don't put static platform data into the drivers; because static platform data is platform specific, and therefore belongs with the platform. An OF translator is different from a static pdata because it is driver specific code instead of platform specific code. Platform specific code is only applicable for the platform, so of course it belongs with the platform code. Driver specific code belongs with the driver because it isn't applicable to anything else. The whole point of the device tree is that it allows driver specific code to be written that understands the data describing the device instead of having a programmer hard code it. > >> So, in my opinion, translation code must: >> 1. be *tiny* > > Yeah, dream on. ;-) It's tiny when all you have is of_get_property(), > I'd like to see the code when you'll have GPIOs, IRQs, and platform- > specific fixups. It's still pretty tiny, because it still needs to populate a pdata structure. 99% of the time there won't be any platform specific fixups either.... In the odd case when there *are* platform specific hacks, then of course the pdata should be created by platform code, and not driver code. One way to do this is to have platform code hook into the notifier call chain for a bus and watch for devices it needs to meddle with. When one shows up, register the custom pdata before the driver gets probed. But that is the special case, which doesn't need to impact the common case. > You might say that at24 doesn't need that stuff, but it does. > Suppose AT24's WP pin is connected to a GPIO, and without > 'read-only' property I'd like the driver to pull the pin low, > and vice versa: with 'read-only' specifier, WP should be tied > high. Or if WP is controlled by a switch/jumper, GPIO can be > used to read current WP state. I still don't see a problem. If it can be described in pdata, then a translator function can populate it from the device tree data. It still isn't huge. Besides, just because it *might* someday become huge doesn't mean that it should be segregated into another file now. It can always be moved later if it becomes too big. >> -- should be trivial to add to a driver without impacting >> common code > > This is doable, yes. > >> > No matter how small the OF code is, I believe we shouldn't put it >> > into the generic code. Take a look at mmc_spi case again, it can be >> > easily extended to any arch, because there is no arch-specific stuff, >> > but a "get/put" pattern for platform data. >> >> I'm not disagreeing with you that the arch specific stuff should be >> logically separated from the generic code. But I don't agree that it >> belongs in a separate file. And I also think that the mmc_spi >> implementation uses too much code. There must be a better way. > > I wonder how you'd shrink the mmc_spi bindings, can you elaborate? To start; eliminate all the pdata management code and write some library routines to do that instead. Next, I'd refactor the code to separate out the GPIO handling stuff because the GPIO handling really isn't related to OF at all (that code could just as easily be used by a static pdata structure definition). All that should be left is the meat of the mmc_spi_get_pdata() function which parses the device tree and populates pdata. I agree that the infrastructure to do what I'm suggesting doesn't exist yet; but I say this because I think it is the direction that device tree support needs to go. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
