Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
On Thu, Oct 8, 2009 at 4:20 PM, Anton Vorontsov avoront...@ru.mvista.com 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
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
On Fri, 2009-10-09 at 07:14 +0200, Wolfram Sang wrote: And while doing this and figuring the pro/cons of those methods, I stumbled over this commit: gpio: pca953x: Get platform_data from OpenFirmware (1965d30356c1c65660ba3330927671cfe81acdd5) Aside from any issues you have with the properties themselves, what's your take on this approach? Personally, I just got tired of waiting for someone else to solve the pdata/OF problem. So I submitted that commit as an attempt at something very simple and unobtrusive to the device driver itself. It seems pretty clean to me, but I'm curious to see if others have any better ideas. - Nate -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
On Fri, Oct 9, 2009 at 8:01 AM, Nate Case nc...@xes-inc.com wrote:
On Thu, 2009-10-08 at 23:40 -0600, Grant Likely wrote:
For your future reference, patches that look at the device tree should
also cc: devicetree-disc...@lists.ozlabs.org so that new bindings can
be reviewed and common mistakes can be avoided. It is expected that
new device tree bindings are accompanied with documentation describing
what the binding is for and how it should be used (see
Documentation/powerpc/dts-bindings).
I know this change is already in mainline, but can you please post the
device tree fragment that you're using to describe this chip? I want
to make sure we don't get stuck with things in the kernel that will be
hard to maintain in the long term.
Hi Grant,
Sorry for neglecting to include devicetree-discuss on that one. I was
in fact aware of this list, and subscribe to it. I really just forgot
in this case.
No worries, it happens.
I also have a documentation patch for it that went along with it, but it
wasn't ready in time and so it's been sitting in our local patch queue.
I can submit that soon, but it probably makes sense for Wolfram to
voice whatever his concerns were about questionable properties before
I document what's there.
Yes, please post it as soon as you can.
Here's an example device tree node for this case:
gpio1: g...@18 {
compatible = nxp,pca9557;
reg = 0x18;
#gpio-cells = 2;
gpio-controller;
polarity = 0x00;
};
In this case, the linux,gpio-base property wasn't specified. But, the
use case is identical to the pdata-gpio_base field. polarity is used
for specifying polarity inversion for each line, and is in the same
format of the 'polarity inversion' register on the chip. My reasoning
in the property naming was as follows:
linux,gpio-base: Linux-specific as it relates to internal GPIO
numbering. So, it's prefixed with linux,
This would be the 'questionable' property. :-) The device tree is
supposed to be OS agnostic, so I get the heebee geebees when I see new
'linux,blah' properties defined because it means Linux internal
implementation details are getting leaked out into the data blob. The
problem leakage is that the device tree should not be impacted by
internal Linux code changes.
In this particular case, specifying the exact GPIO base number doesn't
really belong in the device tree because Linux is able to assign and
manage GPIO numbers on its own just like all other OF GPIO controller
drivers currently do. In fact, that goes for pretty much all
enumeration, not just GPIO. In general, a particular device instance
(uart, gpio, phy, whatever) should be resolved from its node in the
device tree, and not by some arbitrary number assigned by the .dts
author. The problem with arbitrary numbers is they don't expose the
linkage between nodes in the same way a 'phandle' does (A phandle can
be searched for. An arbitrary number assignment, good luck?), and
they don't expose intended usage (its just a meaningless number, and
it only works because userspace just happens to 'agree' to use the
same number).
polarity: Dictated by how hardware is wired up, so it's needed
regardless of the OS.
Typically GPIO drivers have been handling this by using #gpio-cells
set to 2, and using the 2nd cell for flags. The priority can be
encoded as a flag.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-embedded in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
Aside from any issues you have with the properties themselves, what's your take on this approach? Well, my approach for AT24 looked very similar to your approach. In fact, even the motivation was the same as yours :) Well, the outcome of this is the current thread and no definite solution yet. The archdata surely helps for this issue, it just seems that a bit more generalization is needed. Kind regards, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
On Fri, Oct 9, 2009 at 7:43 AM, Nate Case nc...@xes-inc.com wrote: On Fri, 2009-10-09 at 07:14 +0200, Wolfram Sang wrote: And while doing this and figuring the pro/cons of those methods, I stumbled over this commit: gpio: pca953x: Get platform_data from OpenFirmware (1965d30356c1c65660ba3330927671cfe81acdd5) Aside from any issues you have with the properties themselves, what's your take on this approach? As I mentioned in an earlier email, I don't think quite the right form has been found yet, but I like the direction things are moving. Personally, I just got tired of waiting for someone else to solve the pdata/OF problem. So I submitted that commit as an attempt at something very simple and unobtrusive to the device driver itself. It seems pretty clean to me, but I'm curious to see if others have any better ideas. Yup, that's good. Between Anton's, Wolfram's and your work things are going the right way. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
I can submit that soon, but it probably makes sense for Wolfram to voice whatever his concerns were about questionable properties before I document what's there. Please don't feel offended. The things I noticed are: a) no documentation b) 'polarity' is a direct mapping to the register which IMO is a hint to look closer. I haven't checked in detil, but maybe the active_low-flag could be used for this? I mainly got alarmed that properties were mainlined without being reviewed; as the device-tree is based on convention (which is hard to change afterwards), I try to make sure this will not so easily happen again (thus the get_maintainer-patch on lkml). Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
On Thu, Oct 8, 2009 at 9:10 AM, Anton Vorontsov avoront...@ru.mvista.com wrote: On Thu, Oct 08, 2009 at 08:53:46AM -0600, Grant Likely wrote: [...] Please don't. It is such a small amount of code, It's *always* a small amound of code, at a start. Then we get floppy disk drivers and the tty layer. ;-) Holy straw man argument Batman! 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. [...] Driver writers shouldn't have to write anything more than a tiny function to populate pdata from the device tree. Managing that pdata instance needs to be done with common infrastructure (but I don't have a firm idea about how it should look yet). In the mean time I think Wolfram's approach has lower impact. 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. So, in my opinion, translation code must: 1. be *tiny* -- should be trivial to add to a driver without impacting common code 2. live with the driver that it translates data for; ideally in the same .c file for drivers that are small. 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. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line unsubscribe linux-embedded in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
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. So, in my opinion, translation code must: 1. be *tiny* -- should be trivial to add to a driver without impacting common code 2. live with the driver that it translates data for; ideally in the same .c file for drivers that are small. I am with Grant on these points. It is more than just PPC. 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. Will check this tomorrow. -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [RFC] misc/at24: add experimental OF support for the generic eeprom driver
Will check this tomorrow. And while doing this and figuring the pro/cons of those methods, I stumbled over this commit: gpio: pca953x: Get platform_data from OpenFirmware (1965d30356c1c65660ba3330927671cfe81acdd5) It looks to me that it missed all people involved in OF/DT-development and now we have undocumented and IMO questionable properties in the kernel. Conclusions I draw: a) we better solve the pdata-problem rather sooner than later ;) b) we need to spread the word about devicetree-discuss c) more documentation may help, too I know, 'send patches'... Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
