| Message ID | 48050ec08d248a2a10b4f5faf6cac6b214041ebe.1319658296.git.marvin24@gmx.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM: > This adds device tree support to the nvec driver. By using this method > it is no longer necessary to specify platform data through a board > file. You should document the binding in Documentation/devicetree/bindings. > @@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev) > #define tegra_nvec_resume NULL > #endif > > +#if defined(CONFIG_OF) I think you can just remove the ifdef and always include this code. Yes, it'll result in slightly more rodata when !CONFIG_OF, but !CONFIG_OF isn't going to exist or be useful for Tegra for that much longer. > +/* Match table for of_platform binding */ > +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = { > + { .compatible = "nvidia,nvec", }, I'm not sure that nvidia,nvec is the right value, but need a little more background. It's my understanding that how this works is a little micro-controller exists on the board, handles various devices like the keyboard, and sends data to Tegra by making I2C master transactions. Isn't it the case that the micro-controller (or at least the SW running on it) is board-specific, and the same for the I2C protocol? If so, nvidia,nvec is a little generic; we probably need to name it compal,paz00-ec or something like that? Either way, we should probably include some kind of version number in the compatible property so we can support upgrades to the protocol if needed.
On Thu, Oct 27, 2011 at 12:17:25PM -0700, Stephen Warren wrote: > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM: > > This adds device tree support to the nvec driver. By using this method > > it is no longer necessary to specify platform data through a board > > file. > > You should document the binding in Documentation/devicetree/bindings. > > > @@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev) > > #define tegra_nvec_resume NULL > > #endif > > > > +#if defined(CONFIG_OF) > > I think you can just remove the ifdef and always include this code. Yes, it'll > result in slightly more rodata when !CONFIG_OF, but !CONFIG_OF isn't going to > exist or be useful for Tegra for that much longer. Often things do not actually build anymore without CONFIG_OF -- For example, the code in -next failed to build a month ago or so (don't know if that's still the case, though). > > +/* Match table for of_platform binding */ > > +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = { > > + { .compatible = "nvidia,nvec", }, > > I'm not sure that nvidia,nvec is the right value, but need a little more > background. > > It's my understanding that how this works is a little micro-controller > exists on the board, handles various devices like the keyboard, and sends > data to Tegra by making I2C master transactions. Isn't it the case that > the micro-controller (or at least the SW running on it) is board-specific, > and the same for the I2C protocol? If so, nvidia,nvec is a little generic; > we probably need to name it compal,paz00-ec or something like that? nvec means Nvidia Embedded Controller, and the protocol is not device-specific anyway. There are some device-specific things, the controller has some commands reserved for OEM usage. We do not use those yet. For an example of a non-paz00 device: The Advent Vega tablet and similar (those with boards called "shuttle") also use nvec. If you want to know details, you could search someone at your company who knows more about it. There should be a few people knowing things about nvec.
Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren: > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM: > > This adds device tree support to the nvec driver. By using this method > > it is no longer necessary to specify platform data through a board > > file. > > You should document the binding in Documentation/devicetree/bindings. oh, I feared that ... Will go in to v3. > > @@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev) > > > > #define tegra_nvec_resume NULL > > #endif > > > > +#if defined(CONFIG_OF) > > I think you can just remove the ifdef and always include this code. Yes, it'll > result in slightly more rodata when !CONFIG_OF, but !CONFIG_OF isn't going to > exist or be useful for Tegra for that much longer. ok, I will check if it causes build regressions first. > > > +/* Match table for of_platform binding */ > > +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = { > > + { .compatible = "nvidia,nvec", }, > > I'm not sure that nvidia,nvec is the right value, but need a little more > background. > > It's my understanding that how this works is a little micro-controller > exists on the board, handles various devices like the keyboard, and sends > data to Tegra by making I2C master transactions. Isn't it the case that > the micro-controller (or at least the SW running on it) is board-specific, > and the same for the I2C protocol? If so, nvidia,nvec is a little generic; > we probably need to name it compal,paz00-ec or something like that? The firmware (for the 8051 mc inside the keyboard controller) is likely made by Compal, but as Julian already said, the EC protocol definition is very likely from NVIDIA itself. Compal just implemented it for the master. You may refer to <http://nv- tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff;h=12114faf442a8c6aac81a9702712077364db0e82> Also this protocol is not board specific as many first generation boards/device use it, so "nvidia,nvec" should be correct here. > Either way, we should probably include some kind of version number in > the compatible property so we can support upgrades to the protocol if > needed. You may ask your colleagues on that topic, but it seems that the protocol is dead already, e.g. it wasn't implemented for the new-world kernels (>= .36) anymore. Marc -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marc Dietrich wrote at Friday, October 28, 2011 5:02 AM: > Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren: > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM: > > > This adds device tree support to the nvec driver. By using this method > > > it is no longer necessary to specify platform data through a board > > > file. ... > > > +/* Match table for of_platform binding */ > > > +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = { > > > + { .compatible = "nvidia,nvec", }, > > > > I'm not sure that nvidia,nvec is the right value, but need a little more > > background. > > > > It's my understanding that how this works is a little micro-controller > > exists on the board, handles various devices like the keyboard, and sends > > data to Tegra by making I2C master transactions. Isn't it the case that > > the micro-controller (or at least the SW running on it) is board-specific, > > and the same for the I2C protocol? If so, nvidia,nvec is a little generic; > > we probably need to name it compal,paz00-ec or something like that? > > The firmware (for the 8051 mc inside the keyboard controller) is likely made by > Compal, but as Julian already said, the EC protocol definition is very likely from > NVIDIA itself. Compal just implemented it for the master. You may refer to > <http://nv- > tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff;h=12114faf442a8c6aac81a9702712077364db0e82> > Also this protocol is not board specific as many first generation boards/device use > it, so "nvidia,nvec" should be correct here. > > > Either way, we should probably include some kind of version number in > > the compatible property so we can support upgrades to the protocol if > > needed. > > You may ask your colleagues on that topic, but it seems that the protocol is dead > already, e.g. it wasn't implemented for the new-world kernels (>= .36) anymore. OK, I asked internally and it sounds like this is /probably/ standardized. That said, there are apparently some OEMs who did change the protocol and do something slightly different. I'm trying to confirm whether PAZ00 was one of them. I guess not if PAZ00 works with the standard driver that you linked to. So the good news is that there's an internal specification for this protocol, and we might be able to release it. I'll let you know if/when there are updates on this. I'd like to call this "nvidia,nvec-1.0" to version this compatible property; that's the specification version in the latest document that I saw. While we do seemed to have abandoned this approach, I want to make sure this is extensible if someone suddenly decides to go back to it and creates a 2.0 in the future. Does that seem reasonable?
On Friday 28 October 2011 09:56:41 you wrote: > Marc Dietrich wrote at Friday, October 28, 2011 5:02 AM: > > Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren: > > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM: > > > > This adds device tree support to the nvec driver. By using this > > > > method it is no longer necessary to specify platform data > > > > through a board file. > > ... > > > > > +/* Match table for of_platform binding */ > > > > +static const struct of_device_id nvidia_nvec_of_match[] > > > > __devinitconst = { + { .compatible = "nvidia,nvec", }, > > > > > > I'm not sure that nvidia,nvec is the right value, but need a little > > > more background. > > > > > > It's my understanding that how this works is a little > > > micro-controller > > > exists on the board, handles various devices like the keyboard, and > > > sends data to Tegra by making I2C master transactions. Isn't it the > > > case that the micro-controller (or at least the SW running on it) > > > is board-specific, and the same for the I2C protocol? If so, > > > nvidia,nvec is a little generic; we probably need to name it > > > compal,paz00-ec or something like that?> > > The firmware (for the 8051 mc inside the keyboard controller) is likely > > made by Compal, but as Julian already said, the EC protocol definition > > is very likely from NVIDIA itself. Compal just implemented it for the > > master. You may refer to <http://nv- > > tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff;h=12114faf442a8c6a > > ac81a9702712077364db0e82> Also this protocol is not board specific as > > many first generation boards/device use it, so "nvidia,nvec" should be > > correct here. > > > > > Either way, we should probably include some kind of version number > > > in > > > the compatible property so we can support upgrades to the protocol > > > if > > > needed. > > > > You may ask your colleagues on that topic, but it seems that the > > protocol is dead already, e.g. it wasn't implemented for the new-world > > kernels (>= .36) anymore. > OK, I asked internally and it sounds like this is /probably/ standardized. > > That said, there are apparently some OEMs who did change the protocol > and do something slightly different. I'm trying to confirm whether PAZ00 > was one of them. I guess not if PAZ00 works with the standard driver that > you linked to. There are so called OEM commands which we will move to a board specific nvec file (e.g. nvec_paz00.c). We haven't got the chance to test it on other boards using it yet (e.g. toshiba folio, advent vega, ...). The tablets are mostly using it for power control and maybe also leds/switches. I don't think any other board uses keyboard / mouse functions. > So the good news is that there's an internal specification for this > protocol, and we might be able to release it. I'll let you know if/when > there are updates on this. The original source is well documentated already, but additional info is always welcome ;-) > I'd like to call this "nvidia,nvec-1.0" to version this compatible > property; that's the specification version in the latest document that > I saw. While we do seemed to have abandoned this approach, I want to > make sure this is extensible if someone suddenly decides to go back to > it and creates a 2.0 in the future. Does that seem reasonable? mmh, I can't see why we should add it now. There is no V2 I can see in my limited view. If your company plans to expand the protocol you can either enhance our driver or create a new one (nvec2), which can add a nvidia,nvec-2 compatibility property (we can also change ours to nvidia,nvec-1 at the same time, but that's not required). Marc -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marc Dietrich wrote at Sunday, October 30, 2011 2:58 PM: > On Friday 28 October 2011 09:56:41 you wrote: > > Marc Dietrich wrote at Friday, October 28, 2011 5:02 AM: > > > Am Donnerstag, 27. Oktober 2011, 12:17:25 schrieb Stephen Warren: > > > > Marc Dietrich wrote at Wednesday, October 26, 2011 1:59 PM: > > > > > This adds device tree support to the nvec driver. By using this > > > > > method it is no longer necessary to specify platform data > > > > > through a board file. > > > > ... > > > > > > > +/* Match table for of_platform binding */ > > > > > +static const struct of_device_id nvidia_nvec_of_match[] > > > > > __devinitconst = { + { .compatible = "nvidia,nvec", }, ... > > I'd like to call this "nvidia,nvec-1.0" to version this compatible > > property; that's the specification version in the latest document that > > I saw. While we do seemed to have abandoned this approach, I want to > > make sure this is extensible if someone suddenly decides to go back to > > it and creates a 2.0 in the future. Does that seem reasonable? > > mmh, I can't see why we should add it now. There is no V2 I can see in my > limited view. If your company plans to expand the protocol you can either > enhance our driver or create a new one (nvec2), which can add a nvidia,nvec-2 > compatibility property (we can also change ours to nvidia,nvec-1 at the same > time, but that's not required). We can't rename anything in DT once we've started using it; if we release a new kernel that changes (rather than just adds to) the compatible values it supports, that'd cause old DT files to cease to operate against the new kernel. I suppose nvidia,nvec is fine. We can always add nvidia,nvec2 if we do rev the protocol, and have the existing unnumbered value implicitly be version 1. > > That said, there are apparently some OEMs who did change the protocol > > and do something slightly different. I'm trying to confirm whether PAZ00 > > was one of them. I guess not if PAZ00 works with the standard driver that > > you linked to. > > There are so called OEM commands which we will move to a board specific nvec > file (e.g. nvec_paz00.c). We haven't got the chance to test it on other boards > using it yet (e.g. toshiba folio, advent vega, ...). The tablets are mostly > using it for power control and maybe also leds/switches. I don't think any > other board uses keyboard / mouse functions. It sounded like some OEMS had used a completely different protocol rather than just making use of the OEM commands. Still, it isn't entirely clear whether that's true yet. I don't think it impacts this driver/board, so we can just ignore this for now.
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c index fb0f095..731eeeb 100644 --- a/drivers/staging/nvec/nvec.c +++ b/drivers/staging/nvec/nvec.c @@ -26,6 +26,8 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/irq.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/list.h> #include <linux/mfd/core.h> #include <linux/mutex.h> @@ -35,6 +37,8 @@ #include <linux/spinlock.h> #include <linux/workqueue.h> +#include <asm/byteorder.h> + #include <mach/clk.h> #include <mach/iomap.h> @@ -718,6 +722,7 @@ static int __devinit tegra_nvec_probe(struct platform_device *pdev) struct resource *res; struct resource *iomem; void __iomem *base; + const unsigned int *prop; nvec = kzalloc(sizeof(struct nvec_chip), GFP_KERNEL); if (nvec == NULL) { @@ -726,8 +731,26 @@ static int __devinit tegra_nvec_probe(struct platform_device *pdev) } platform_set_drvdata(pdev, nvec); nvec->dev = &pdev->dev; - nvec->gpio = pdata->gpio; - nvec->i2c_addr = pdata->i2c_addr; + + if (pdata) { + nvec->gpio = pdata->gpio; + nvec->i2c_addr = pdata->i2c_addr; + } else if (nvec->dev->of_node) { + nvec->gpio = of_get_named_gpio(nvec->dev->of_node, "request-gpios", 0); + if (nvec->gpio < 0) { + dev_err(&pdev->dev, "no gpio specified"); + goto failed; + } + prop = of_get_property(nvec->dev->of_node, "slave-addr", NULL); + if (!prop) { + dev_err(&pdev->dev, "no i2c address specified"); + goto failed; + } + nvec->i2c_addr = be32_to_cpup(prop); + } else { + dev_err(&pdev->dev, "no platform data\n"); + goto failed; + } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { @@ -892,6 +915,17 @@ static int tegra_nvec_resume(struct platform_device *pdev) #define tegra_nvec_resume NULL #endif +#if defined(CONFIG_OF) +/* Match table for of_platform binding */ +static const struct of_device_id nvidia_nvec_of_match[] __devinitconst = { + { .compatible = "nvidia,nvec", }, + {}, +}; +MODULE_DEVICE_TABLE(of, nvidia_nvec_of_match); +#else +#define nvidia_nvec_of_match NULL +#endif + static struct platform_driver nvec_device_driver = { .probe = tegra_nvec_probe, .remove = __devexit_p(tegra_nvec_remove), @@ -900,6 +934,7 @@ static struct platform_driver nvec_device_driver = { .driver = { .name = "nvec", .owner = THIS_MODULE, + .of_match_table = nvidia_nvec_of_match, } };
This adds device tree support to the nvec driver. By using this method it is no longer necessary to specify platform data through a board file. Cc: devel@linuxdriverproject.org Cc: Julian Andres Klode <jak@jak-linux.org> Signed-off-by: Marc Dietrich <marvin24@gmx.de> --- drivers/staging/nvec/nvec.c | 39 +++++++++++++++++++++++++++++++++++++-- 1 files changed, 37 insertions(+), 2 deletions(-)