| Message ID | 1384465609-26485-3-git-send-email-marek@goldelico.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thursday 14 November 2013, Marek Belisko wrote: > + if (node && !pdata) { > + pdata = devm_kzalloc(dev, sizeof(struct bmp085_platform_data), > + GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + } > + I generally recommend against allocating platform data from inside the driver, as this requires more code and more memory than just adding fields to the driver-specific data structure and copying over the fields from either DT or the supplied platform data, depending on which one is used. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 14, 2013 at 09:46:48PM +0000, Marek Belisko wrote: > Signed-off-by: Marek Belisko <marek@goldelico.com> > --- > Documentation/devicetree/bindings/misc/bmp085.txt | 8 ++++ > drivers/misc/bmp085.c | 53 ++++++++++++++++++++--- > 2 files changed, 55 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/misc/bmp085.txt b/Documentation/devicetree/bindings/misc/bmp085.txt > index 91dfda2..c6033d5 100644 > --- a/Documentation/devicetree/bindings/misc/bmp085.txt > +++ b/Documentation/devicetree/bindings/misc/bmp085.txt > @@ -8,6 +8,9 @@ Optional properties: > - temp-measurement-period: temperature measurement period (milliseconds) > - default-oversampling: default oversampling value to be used at startup, > value range is 0-3 with rising sensitivity. > +- gpio: if device is using EOC irq line gpio can be specified to setup interrupt > + handling > +- irq: interrupt with no gpio > > Example: > > @@ -17,4 +20,9 @@ pressure@77 { > chip-id = <10>; > temp-measurement-period = <100>; > default-oversampling = <2>; > + gpio = <&gpio0 17 0>; > + > + OR > + > + irq = <75>; There's some contention over the description of gpio-based IRQs in DT. >From the point of view of the device there is a logical IRQ output; the fact that this happens to be wired up to a GPIO pin that can happen to generate interrupts isn't anything to do with the device itself. There are plenty of device we have now whose interrupt lines could be wired to GPIOs. I see no reason to extend their bindings to support explicit GPIOs for IRQs, and I see no reason the driver should have to handle this. It would be far nicer for the device binding to just have the interrupts property, and for the gpio controller to act as an interrupt-controller, with the appropriate pin management. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 15 November 2013, Mark Rutland wrote: > There's some contention over the description of gpio-based IRQs in DT. > From the point of view of the device there is a logical IRQ output; the > fact that this happens to be wired up to a GPIO pin that can happen to > generate interrupts isn't anything to do with the device itself. There > are plenty of device we have now whose interrupt lines could be wired to > GPIOs. I see no reason to extend their bindings to support explicit > GPIOs for IRQs, and I see no reason the driver should have to handle > this. > > It would be far nicer for the device binding to just have the interrupts > property, and for the gpio controller to act as an interrupt-controller, > with the appropriate pin management. Yes, agreed. I missed this point in my review: the GPIO is used only as an interrupt pin here, so there is no reason to know the GPIO number. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Fri, Nov 15, 2013 at 4:30 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Nov 14, 2013 at 09:46:48PM +0000, Marek Belisko wrote: >> Signed-off-by: Marek Belisko <marek@goldelico.com> >> --- >> Documentation/devicetree/bindings/misc/bmp085.txt | 8 ++++ >> drivers/misc/bmp085.c | 53 ++++++++++++++++++++--- >> 2 files changed, 55 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/misc/bmp085.txt b/Documentation/devicetree/bindings/misc/bmp085.txt >> index 91dfda2..c6033d5 100644 >> --- a/Documentation/devicetree/bindings/misc/bmp085.txt >> +++ b/Documentation/devicetree/bindings/misc/bmp085.txt >> @@ -8,6 +8,9 @@ Optional properties: >> - temp-measurement-period: temperature measurement period (milliseconds) >> - default-oversampling: default oversampling value to be used at startup, >> value range is 0-3 with rising sensitivity. >> +- gpio: if device is using EOC irq line gpio can be specified to setup interrupt >> + handling >> +- irq: interrupt with no gpio >> >> Example: >> >> @@ -17,4 +20,9 @@ pressure@77 { >> chip-id = <10>; >> temp-measurement-period = <100>; >> default-oversampling = <2>; >> + gpio = <&gpio0 17 0>; >> + >> + OR >> + >> + irq = <75>; > > There's some contention over the description of gpio-based IRQs in DT. > From the point of view of the device there is a logical IRQ output; the > fact that this happens to be wired up to a GPIO pin that can happen to > generate interrupts isn't anything to do with the device itself. There > are plenty of device we have now whose interrupt lines could be wired to > GPIOs. I see no reason to extend their bindings to support explicit > GPIOs for IRQs, and I see no reason the driver should have to handle > this. > > It would be far nicer for the device binding to just have the interrupts > property, and for the gpio controller to act as an interrupt-controller, > with the appropriate pin management. OK. Can you please give me some example in current bindings tree? Something like in: Documentation/devicetree/bindings/fb/mxsfb.txt ? So if I understand correctly we define only property interrupts = <xxx> which will be exact interrupt number for cpu gpio connected to bmp085 irq line. > > Thanks, > Mark. Thanks, marek
On Monday 18 November 2013, Belisko Marek wrote: > > It would be far nicer for the device binding to just have the interrupts > > property, and for the gpio controller to act as an interrupt-controller, > > with the appropriate pin management. > OK. Can you please give me some example in current bindings tree? > Something like in: > Documentation/devicetree/bindings/fb/mxsfb.txt ? So if I understand > correctly we define only property > interrupts = <xxx> which will be exact interrupt number for cpu gpio > connected to bmp085 irq line. > > See Documentation/devicetree/bindings/gpio/8xxx_gpio.txt for an example of a device whose interrupt line is connected to a gpio controller. The key here is to set the "interrupt-parent" to the gpio node and have the irq specifier define an interrupt local to that node. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ trimmed Cc: list for DT ] [ is the bindings/gpio/8xxx_gpio.txt document incomplete? ] On Mon, Nov 18, 2013 at 12:38 +0100, Arnd Bergmann wrote: > > See Documentation/devicetree/bindings/gpio/8xxx_gpio.txt for an > example of a device whose interrupt line is connected to a gpio > controller. The key here is to set the "interrupt-parent" to > the gpio node and have the irq specifier define an interrupt > local to that node. I was wondering whether the gpio-controller nodes lack both the 'interrupt-controller' boolean flag as well as the '#interrupt-cells = <2>' property. The former may be optional, I'm not certain there. But the latter should be a hard requirement without which the 'funkyfpga' references cannot get parsed I'm afraid. Am I missing something? Did I get the names right? Shall I send a patch? Is the code working without those specs since it assumes knowledge that was not specified in the device tree? And would this be against the idea that the binding should be "complete" and independent of who is interpreting the data? Or is the binding document just incomplete? virtually yours Gerhard Sittig
On Monday 18 November 2013, Gerhard Sittig wrote: > [ trimmed Cc: list for DT ] > [ is the bindings/gpio/8xxx_gpio.txt document incomplete? ] > > On Mon, Nov 18, 2013 at 12:38 +0100, Arnd Bergmann wrote: > > > > See Documentation/devicetree/bindings/gpio/8xxx_gpio.txt for an > > example of a device whose interrupt line is connected to a gpio > > controller. The key here is to set the "interrupt-parent" to > > the gpio node and have the irq specifier define an interrupt > > local to that node. > > I was wondering whether the gpio-controller nodes lack both the > 'interrupt-controller' boolean flag as well as the > '#interrupt-cells = <2>' property. The former may be optional, > I'm not certain there. But the latter should be a hard > requirement without which the 'funkyfpga' references cannot get > parsed I'm afraid. You are absolutely right, I picked an example that was actually wrong. > Am I missing something? Did I get the names right? Shall I send > a patch? Is the code working without those specs since it > assumes knowledge that was not specified in the device tree? And > would this be against the idea that the binding should be > "complete" and independent of who is interpreting the data? Or > is the binding document just incomplete? I think the problem is that this particular controller is never used in "interrupts" properties, so nobody noticed the mistake. It probably still works for gpio references and drivers using gpio_to_irq on those numbers. It would be nice if you can send a patch to add those as optional properties to the binding and the example. We can't really make them mandatory properties now because that would make all existing dts files with this controller invalid. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 18, 2013 at 23:28 +0100, Arnd Bergmann wrote: > > [ ... incomplete 8xxx gpio binding document ... ] > > You are absolutely right, I picked an example that was actually > wrong. > > [ ... ] > > I think the problem is that this particular controller is never > used in "interrupts" properties, so nobody noticed the mistake. > It probably still works for gpio references and drivers using > gpio_to_irq on those numbers. Actually that very GPIO module _is_ being used as an IRQ controller as well. The MPC5121 is one of those (not explicitly listed) compatibles. The mpc5121.dtsi correctly declares the GPIO module as both a GPIO as well as an IRQ controller, and the ac14xx.dts board does use GPIO lines as IRQs. Which suggests that the issue is just incomplete documentation. And that an incomplete binding document does not prevent correct use of the binding. :) And more importantly that the use of DT is so intuitive that people get things right without looking at the docs. :-] virtually yours Gerhard Sittig
diff --git a/Documentation/devicetree/bindings/misc/bmp085.txt b/Documentation/devicetree/bindings/misc/bmp085.txt index 91dfda2..c6033d5 100644 --- a/Documentation/devicetree/bindings/misc/bmp085.txt +++ b/Documentation/devicetree/bindings/misc/bmp085.txt @@ -8,6 +8,9 @@ Optional properties: - temp-measurement-period: temperature measurement period (milliseconds) - default-oversampling: default oversampling value to be used at startup, value range is 0-3 with rising sensitivity. +- gpio: if device is using EOC irq line gpio can be specified to setup interrupt + handling +- irq: interrupt with no gpio Example: @@ -17,4 +20,9 @@ pressure@77 { chip-id = <10>; temp-measurement-period = <100>; default-oversampling = <2>; + gpio = <&gpio0 17 0>; + + OR + + irq = <75>; }; diff --git a/drivers/misc/bmp085.c b/drivers/misc/bmp085.c index 1510a7b..9792ce2 100644 --- a/drivers/misc/bmp085.c +++ b/drivers/misc/bmp085.c @@ -50,6 +50,7 @@ #include <linux/init.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/of_gpio.h> #include "bmp085.h" #include <linux/interrupt.h> #include <linux/completion.h> @@ -396,7 +397,8 @@ int bmp085_detect(struct device *dev) } EXPORT_SYMBOL_GPL(bmp085_detect); -static void bmp085_get_of_properties(struct bmp085_data *data) +static void bmp085_get_of_properties(struct bmp085_data *data, + struct bmp085_platform_data *pdata) { #ifdef CONFIG_OF struct device_node *np = data->dev->of_node; @@ -413,12 +415,18 @@ static void bmp085_get_of_properties(struct bmp085_data *data) if (!of_property_read_u32(np, "default-oversampling", &prop)) data->oversampling_setting = prop & 0xff; + + pdata->gpio = of_get_named_gpio(np, "gpio", 0); + of_property_read_u32(np, "irq", &pdata->irq); #endif } -static int bmp085_init_client(struct bmp085_data *data) +static int bmp085_init_client(struct device *dev, struct bmp085_data *data) { int status = bmp085_read_calibration_data(data); + struct bmp085_platform_data *pdata = dev->platform_data; + struct device_node *node = dev->of_node; + int err; if (status < 0) return status; @@ -429,11 +437,46 @@ static int bmp085_init_client(struct bmp085_data *data) data->temp_measurement_period = 1*HZ; data->oversampling_setting = 3; - bmp085_get_of_properties(data); + /* parse DT to get platform data */ + if (node && !pdata) { + pdata = devm_kzalloc(dev, sizeof(struct bmp085_platform_data), + GFP_KERNEL); + if (!pdata) + return -ENOMEM; + } + + bmp085_get_of_properties(data, pdata); + + if (gpio_is_valid(pdata->gpio)) { + err = devm_gpio_request(dev, pdata->gpio, "bmp085_eoc_irq"); + if (err) + goto exit_free; + err = gpio_direction_input(pdata->gpio); + if (err) + goto exit_free; + data->irq = gpio_to_irq(pdata->gpio); + data->gpio = pdata->gpio; + } else { + if (pdata->irq > 0) + data->irq = pdata->irq; + else + data->irq = 0; + data->gpio = -EINVAL; + } + if (data->irq > 0) { + err = request_any_context_irq(data->irq, bmp085_eoc_isr, + IRQF_TRIGGER_RISING, "bmp085", + data); + if (err < 0) + goto exit_free; + } mutex_init(&data->lock); return 0; + +exit_free: + return err; } struct regmap_config bmp085_regmap_config = { @@ -445,7 +488,6 @@ EXPORT_SYMBOL_GPL(bmp085_regmap_config); int bmp085_probe(struct device *dev, struct regmap *regmap) { struct bmp085_data *data; - struct bmp085_platform_data *pdata = dev->platform_data; int err = 0; data = kzalloc(sizeof(struct bmp085_data), GFP_KERNEL); @@ -484,7 +526,7 @@ int bmp085_probe(struct device *dev, struct regmap *regmap) data->irq = 0; /* Initialize the BMP085 chip */ - err = bmp085_init_client(data); + err = bmp085_init_client(dev, data); if (err < 0) goto exit_free_irq; @@ -506,7 +548,6 @@ int bmp085_probe(struct device *dev, struct regmap *regmap) exit_free_irq: if (data->irq) free_irq(data->irq, data); -exit_free: kfree(data); exit: return err;
Signed-off-by: Marek Belisko <marek@goldelico.com> --- Documentation/devicetree/bindings/misc/bmp085.txt | 8 ++++ drivers/misc/bmp085.c | 53 ++++++++++++++++++++--- 2 files changed, 55 insertions(+), 6 deletions(-)