| Message ID | 1255010672-21656-1-git-send-email-w.sang@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Grant Likely |
| Headers | show |
On Thu, Oct 08, 2009 at 04:04:32PM +0200, Wolfram Sang wrote: > As Anton introduced archdata support, I wondered if this is a suitable way to > handle the platform_data/devicetree_property-dualism (at least for some > drivers). Yes, we handle OF in a similar way for mmc_spi driver. Though, [...] > --- a/drivers/misc/eeprom/at24.c > +++ b/drivers/misc/eeprom/at24.c > @@ -22,6 +22,9 @@ [...] > +#ifdef CONFIG_OF_I2C > +#include <linux/of.h> > +#endif [..] > +#ifdef CONFIG_OF_I2C > +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip) > +{ > + const u32 *val; > + struct device_node *node = dev_archdata_get_node(&client->dev.archdata); > + > + if (node) { > + if (of_get_property(node, "read-only", NULL)) > + chip->flags |= AT24_FLAG_READONLY; > + val = of_get_property(node, "pagesize", NULL); > + if (val) > + chip->page_size = *val; > + } > +} > +#else > +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip) > +{ } > +#endif #ifdefs are ugly in .c files. I'd suggest to move the OF code into a separate file. As an example, see commit 9c43df57910bbba540a6cb5c9132302a9ea5f41a Author: Anton Vorontsov <avorontsov@ru.mvista.com> Date: Tue Dec 30 18:15:28 2008 +0300 mmc_spi: Add support for OpenFirmware bindings Thanks,
On Thu, Oct 8, 2009 at 8:04 AM, Wolfram Sang <w.sang@pengutronix.de> wrote: > As Anton introduced archdata support, I wondered if this is a suitable way to > handle the platform_data/devicetree_property-dualism (at least for some > drivers). I think in general, this is the right direction; but I'm not convinced that the right pattern or form has been found yet. What I don't like on this particular patch is that it still hooks of-specific stuff into an arbitrary point in the probe routine. I'd like to see some pattern for retrieving or populating a platform_data structure when one isn't already provided, and regardless of the data source. So, I guess I'm saying that I agree with the approach, but I think a better pattern would be to factor out all of the platform_data fetching code into a separate function and keep probe() focused on initializing the device based on a pdata structure returned by it. It will take a bit of experimentation to come up with the best form for the pdata fetching function, but it will be better contained if it is all at a single place. g.
On Thu, Oct 8, 2009 at 8:33 AM, Anton Vorontsov <avorontsov@ru.mvista.com> wrote: > On Thu, Oct 08, 2009 at 04:04:32PM +0200, Wolfram Sang wrote: >> As Anton introduced archdata support, I wondered if this is a suitable way to >> handle the platform_data/devicetree_property-dualism (at least for some >> drivers). > > Yes, we handle OF in a similar way for mmc_spi driver. Though, > > [...] >> --- a/drivers/misc/eeprom/at24.c >> +++ b/drivers/misc/eeprom/at24.c >> @@ -22,6 +22,9 @@ > [...] >> +#ifdef CONFIG_OF_I2C >> +#include <linux/of.h> >> +#endif > [..] >> +#ifdef CONFIG_OF_I2C >> +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip) >> +{ >> + const u32 *val; >> + struct device_node *node = dev_archdata_get_node(&client->dev.archdata); >> + >> + if (node) { >> + if (of_get_property(node, "read-only", NULL)) >> + chip->flags |= AT24_FLAG_READONLY; >> + val = of_get_property(node, "pagesize", NULL); >> + if (val) >> + chip->page_size = *val; >> + } >> +} >> +#else >> +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip) >> +{ } >> +#endif > > #ifdefs are ugly in .c files. I'd suggest to move the OF code > into a separate file. As an example, see Please don't. It is such a small amount of code, and I far prefer to see drivers self contained in a single .c file. #ifdefs are fine IMHO when it is a top level block, and not inside a function block. In the example you give, I do like the move toward focusing on the pdata structure; but the patch ads a *lot* of code for something very simple. And then we'll need to do the same think for every driver which will ever be described in the device tree. It's the right direction, but still not right. 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. g.
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. ;-) [...] > 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 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. Thanks,
On Thu, Oct 8, 2009 at 9:10 AM, Anton Vorontsov <avorontsov@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.
> 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.
> I think in general, this is the right direction; but I'm not convinced > that the right pattern or form has been found yet. What I don't like > on this particular patch is that it still hooks of-specific stuff into > an arbitrary point in the probe routine. > > I'd like to see some pattern for retrieving or populating a > platform_data structure when one isn't already provided, and > regardless of the data source. I thought about this, too. I just wondered how many drivers would actually need a 'pdata'-preparation routine before probe, and if this would not cause too much overhead for those who don't. But as you say OF might not the only case where this is needed, then it might be a reasonable choice to have an extra call fot setting up pdata. Then again, if we have preparation routines for OF,UEFI,... for each and every driver, uh, the bloat :( > will take a bit of experimentation to come up with the best form for > the pdata fetching function, but it will be better contained if it is > all at a single place. I might have a try :)
On Thu, Oct 08, 2009 at 09:48:50AM -0600, Grant Likely wrote: > On Thu, Oct 8, 2009 at 9:10 AM, Anton Vorontsov [...] > > 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. 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. 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 :-). > > 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. 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()... > 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. 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. > -- 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? Thanks,
On Thu, Oct 8, 2009 at 2:48 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > >> I think in general, this is the right direction; but I'm not convinced >> that the right pattern or form has been found yet. What I don't like >> on this particular patch is that it still hooks of-specific stuff into >> an arbitrary point in the probe routine. >> >> I'd like to see some pattern for retrieving or populating a >> platform_data structure when one isn't already provided, and >> regardless of the data source. > > I thought about this, too. I just wondered how many drivers would actually need > a 'pdata'-preparation routine before probe, and if this would not cause too > much overhead for those who don't. But as you say OF might not the only case > where this is needed, then it might be a reasonable choice to have an extra > call fot setting up pdata. Then again, if we have preparation routines for > OF,UEFI,... for each and every driver, uh, the bloat :( It shouldn't be too bad.... I'm toying with the idea of a callable library function which is passed in a data table of the available translators. Can be very simple, and the library function would take care of pdata allocation and management. Then the translators can be kept down to the bare minimum of populating the structure. If it is done right, then only the needed translators get compiled in and it won't add any significant size at all to the drivers. >> will take a bit of experimentation to come up with the best form for >> the pdata fetching function, but it will be better contained if it is >> all at a single place. > > I might have a try :) :-)
> 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
On Thu, Oct 8, 2009 at 11:14 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: >> 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. Hi Nate, For your future reference, patches that look at the device tree should also cc: devicetree-discuss@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. Thanks, g.
On Thu, Oct 8, 2009 at 4:20 PM, Anton Vorontsov <avorontsov@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 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.
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
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-discuss@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. 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. Here's an example device tree node for this case: gpio1: gpio@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," polarity: Dictated by how hardware is wired up, so it's needed regardless of the OS. - Nate
On Fri, Oct 9, 2009 at 8:01 AM, Nate Case <ncase@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-discuss@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: gpio@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.
> 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
On Fri, Oct 9, 2009 at 7:43 AM, Nate Case <ncase@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.
> 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
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index db39f4a..4e543e6 100644 --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -22,6 +22,9 @@ #include <linux/jiffies.h> #include <linux/i2c.h> #include <linux/i2c/at24.h> +#ifdef CONFIG_OF_I2C +#include <linux/of.h> +#endif /* * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable. @@ -414,6 +417,25 @@ static ssize_t at24_macc_write(struct memory_accessor *macc, const char *buf, return at24_write(at24, buf, offset, count); } +#ifdef CONFIG_OF_I2C +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip) +{ + const u32 *val; + struct device_node *node = dev_archdata_get_node(&client->dev.archdata); + + if (node) { + if (of_get_property(node, "read-only", NULL)) + chip->flags |= AT24_FLAG_READONLY; + val = of_get_property(node, "pagesize", NULL); + if (val) + chip->page_size = *val; + } +} +#else +static void at24_get_ofdata(struct i2c_client *client, struct at24_platform_data *chip) +{ } +#endif + /*-------------------------------------------------------------------------*/ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) @@ -444,6 +466,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) */ chip.page_size = 1; + /* update chipdata if OF is present */ + at24_get_ofdata(client, &chip); + chip.setup = NULL; chip.context = NULL; }
As Anton introduced archdata support, I wondered if this is a suitable way to handle the platform_data/devicetree_property-dualism (at least for some drivers). If considered suitable, I would document the bindings properly. I really think that pagesize deserves its own property as it is specific to the hardware and I have seen devices with equal name and still having different pagesizes. Not too sure about 'read-only' though, I just copied it from flash-partitions :) Tested on a phyCORE-MPC5200-IO and build-tested on x86. Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> Cc: Grant Likely <grant.likely@secretlab.ca> Cc: Anton Vorontsov <avorontsov@ru.mvista.com> --- drivers/misc/eeprom/at24.c | 25 +++++++++++++++++++++++++ 1 files changed, 25 insertions(+), 0 deletions(-)