* [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order @ 2012-11-22 18:56 Viresh Kumar 2012-11-22 18:56 ` [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver Viresh Kumar ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Viresh Kumar @ 2012-11-22 18:56 UTC (permalink / raw) To: sameo Cc: devicetree-discuss, linux-kernel, spear-devel, lee.jones, Viresh Kumar This helps managing them better and also reduces chances of adding an header file twice. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/mfd/stmpe-spi.c | 2 +- drivers/mfd/stmpe.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c index 9edfe86..f86e3fc 100644 --- a/drivers/mfd/stmpe-spi.c +++ b/drivers/mfd/stmpe-spi.c @@ -7,10 +7,10 @@ * Author: Viresh Kumar <viresh.linux@gmail.com> for ST Microelectronics */ -#include <linux/spi/spi.h> #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/spi/spi.h> #include <linux/types.h> #include "stmpe.h" diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c index 8a4247b..6086481 100644 --- a/drivers/mfd/stmpe.c +++ b/drivers/mfd/stmpe.c @@ -7,15 +7,15 @@ * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson */ -#include <linux/gpio.h> #include <linux/export.h> -#include <linux/kernel.h> +#include <linux/gpio.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/irqdomain.h> +#include <linux/kernel.h> +#include <linux/mfd/core.h> #include <linux/pm.h> #include <linux/slab.h> -#include <linux/mfd/core.h> #include "stmpe.h" static int __stmpe_enable(struct stmpe *stmpe, unsigned int blocks) -- 1.7.12.rc2.18.g61b472e ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver 2012-11-22 18:56 [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Viresh Kumar @ 2012-11-22 18:56 ` Viresh Kumar 2012-11-26 11:15 ` Samuel Ortiz 2012-11-22 18:56 ` [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver Viresh Kumar 2012-11-26 11:16 ` [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Samuel Ortiz 2 siblings, 1 reply; 22+ messages in thread From: Viresh Kumar @ 2012-11-22 18:56 UTC (permalink / raw) To: sameo Cc: devicetree-discuss, linux-kernel, spear-devel, lee.jones, Viresh Kumar Currently, few fields in stmpe_i2c_driver are initialized as: .driver.owner = THIS_MODULE, Group them under {}, like: .driver = { .owner = THIS_MODULE, ... }, Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/mfd/stmpe-i2c.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c index 947a06a..c734dc3 100644 --- a/drivers/mfd/stmpe-i2c.c +++ b/drivers/mfd/stmpe-i2c.c @@ -82,11 +82,13 @@ static const struct i2c_device_id stmpe_i2c_id[] = { MODULE_DEVICE_TABLE(i2c, stmpe_id); static struct i2c_driver stmpe_i2c_driver = { - .driver.name = "stmpe-i2c", - .driver.owner = THIS_MODULE, + .driver = { + .name = "stmpe-i2c", + .owner = THIS_MODULE, #ifdef CONFIG_PM - .driver.pm = &stmpe_dev_pm_ops, + .pm = &stmpe_dev_pm_ops, #endif + }, .probe = stmpe_i2c_probe, .remove = __devexit_p(stmpe_i2c_remove), .id_table = stmpe_i2c_id, -- 1.7.12.rc2.18.g61b472e ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver 2012-11-22 18:56 ` [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver Viresh Kumar @ 2012-11-26 11:15 ` Samuel Ortiz 0 siblings, 0 replies; 22+ messages in thread From: Samuel Ortiz @ 2012-11-26 11:15 UTC (permalink / raw) To: Viresh Kumar; +Cc: devicetree-discuss, linux-kernel, spear-devel, lee.jones Hi Viresh, On Fri, Nov 23, 2012 at 12:26:19AM +0530, Viresh Kumar wrote: > Currently, few fields in stmpe_i2c_driver are initialized as: > .driver.owner = THIS_MODULE, > > Group them under {}, like: > .driver = { > .owner = THIS_MODULE, > ... > }, > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/mfd/stmpe-i2c.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) Applied, thanks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-22 18:56 [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Viresh Kumar 2012-11-22 18:56 ` [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver Viresh Kumar @ 2012-11-22 18:56 ` Viresh Kumar 2012-11-23 9:41 ` Grant Likely 2012-11-23 10:03 ` Lee Jones 2012-11-26 11:16 ` [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Samuel Ortiz 2 siblings, 2 replies; 22+ messages in thread From: Viresh Kumar @ 2012-11-22 18:56 UTC (permalink / raw) To: sameo Cc: devicetree-discuss, linux-kernel, spear-devel, lee.jones, Vipul Kumar Samar, Viresh Kumar From: Vipul Kumar Samar <vipulkumar.samar@st.com> This patch extends existing DT support for stmpe devices. This updates: - DT support from stmpe SPI and I2C drivers - missing header files in stmpe.c - stmpe_of_probe() with pwm, rotator and new bindings. - Bindings are updated in binding document. Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- V2->V3: ------ - Removed sub-modules DT bindings from this patch - Retain original work done by Lee Jones Documentation/devicetree/bindings/mfd/stmpe.txt | 12 ++++++++---- drivers/mfd/stmpe-i2c.c | 15 +++++++++++++++ drivers/mfd/stmpe-spi.c | 15 +++++++++++++++ drivers/mfd/stmpe.c | 21 +++++++++++++++------ 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt index 8f0aeda..8f65c8d 100644 --- a/Documentation/devicetree/bindings/mfd/stmpe.txt +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt @@ -1,13 +1,17 @@ -* STMPE Multi-Functional Device +* ST Microelectronics STMPE Multi-Functional Device + +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad, +touchscreen, adc, pwm, rotator. Required properties: - - compatible : "st,stmpe[811|1601|2401|2403]" - - reg : I2C address of the device + - compatible : "st,stmpe[610|801|811|1601|2401|2403]" + - reg : I2C/SPI address of the device Optional properties: - interrupts : The interrupt outputs from the controller - - interrupt-controller : Marks the device node as an interrupt controller - interrupt-parent : Specifies which IRQ controller we're connected to + - irq-trigger : IRQ trigger to use for the interrupt to the host + - irq-invert-polarity : bool, IRQ line is connected with reversed polarity - i2c-client-wake : Marks the input device as wakable - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c index c734dc3..003c49b 100644 --- a/drivers/mfd/stmpe-i2c.c +++ b/drivers/mfd/stmpe-i2c.c @@ -13,6 +13,7 @@ #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/types.h> #include "stmpe.h" @@ -81,6 +82,19 @@ static const struct i2c_device_id stmpe_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, stmpe_id); +#ifdef CONFIG_OF +static const struct of_device_id stmpe_dt_ids[] = { + { .compatible = "st,stmpe610", .data = &stmpe_i2c_id[0], }, + { .compatible = "st,stmpe801", .data = &stmpe_i2c_id[1], }, + { .compatible = "st,stmpe811", .data = &stmpe_i2c_id[2], }, + { .compatible = "st,stmpe1601", .data = &stmpe_i2c_id[3], }, + { .compatible = "st,stmpe2401", .data = &stmpe_i2c_id[4], }, + { .compatible = "st,stmpe2403", .data = &stmpe_i2c_id[5], }, + { } +}; +MODULE_DEVICE_TABLE(of, stmpe_dt_ids); +#endif + static struct i2c_driver stmpe_i2c_driver = { .driver = { .name = "stmpe-i2c", @@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = { #ifdef CONFIG_PM .pm = &stmpe_dev_pm_ops, #endif + .of_match_table = of_match_ptr(stmpe_dt_ids), }, .probe = stmpe_i2c_probe, .remove = __devexit_p(stmpe_i2c_remove), diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c index f86e3fc..e482f5f 100644 --- a/drivers/mfd/stmpe-spi.c +++ b/drivers/mfd/stmpe-spi.c @@ -10,6 +10,7 @@ #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/spi/spi.h> #include <linux/types.h> #include "stmpe.h" @@ -119,6 +120,19 @@ static const struct spi_device_id stmpe_spi_id[] = { }; MODULE_DEVICE_TABLE(spi, stmpe_id); +#ifdef CONFIG_OF +static const struct of_device_id stmpe_dt_ids[] = { + { .compatible = "st,stmpe610", .data = (void *)STMPE610, }, + { .compatible = "st,stmpe801", .data = (void *)STMPE801, }, + { .compatible = "st,stmpe811", .data = (void *)STMPE811, }, + { .compatible = "st,stmpe1601", .data = (void *)STMPE1601, }, + { .compatible = "st,stmpe2401", .data = (void *)STMPE2401, }, + { .compatible = "st,stmpe2403", .data = (void *)STMPE2403, }, + { } +}; +MODULE_DEVICE_TABLE(of, stmpe_dt_ids); +#endif + static struct spi_driver stmpe_spi_driver = { .driver = { .name = "stmpe-spi", @@ -126,6 +140,7 @@ static struct spi_driver stmpe_spi_driver = { #ifdef CONFIG_PM .pm = &stmpe_dev_pm_ops, #endif + .of_match_table = of_match_ptr(stmpe_dt_ids), }, .probe = stmpe_spi_probe, .remove = __devexit_p(stmpe_spi_remove), diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c index 6086481..e2c0dda 100644 --- a/drivers/mfd/stmpe.c +++ b/drivers/mfd/stmpe.c @@ -7,6 +7,7 @@ * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson */ +#include <linux/err.h> #include <linux/export.h> #include <linux/gpio.h> #include <linux/interrupt.h> @@ -14,6 +15,8 @@ #include <linux/irqdomain.h> #include <linux/kernel.h> #include <linux/mfd/core.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/pm.h> #include <linux/slab.h> #include "stmpe.h" @@ -1028,18 +1031,24 @@ void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata, pdata->autosleep = (pdata->autosleep_timeout) ? true : false; + of_property_read_u32(np, "irq-trigger", &pdata->irq_trigger); + + if (of_property_read_bool(np, "irq-invert-polarity")) + pdata->irq_invert_polarity = true; + for_each_child_of_node(np, child) { if (!strcmp(child->name, "stmpe_gpio")) { pdata->blocks |= STMPE_BLOCK_GPIO; - } - if (!strcmp(child->name, "stmpe_keypad")) { + } else if (!strcmp(child->name, "stmpe_keypad")) { pdata->blocks |= STMPE_BLOCK_KEYPAD; - } - if (!strcmp(child->name, "stmpe_touchscreen")) { + } else if (!strcmp(child->name, "stmpe_touchscreen")) { pdata->blocks |= STMPE_BLOCK_TOUCHSCREEN; - } - if (!strcmp(child->name, "stmpe_adc")) { + } else if (!strcmp(child->name, "stmpe_adc")) { pdata->blocks |= STMPE_BLOCK_ADC; + } else if (!strcmp(child->name, "stmpe_pwm")) { + pdata->blocks |= STMPE_BLOCK_PWM; + } else if (!strcmp(child->name, "stmpe_rotator")) { + pdata->blocks |= STMPE_BLOCK_ROTATOR; } } } -- 1.7.12.rc2.18.g61b472e ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-22 18:56 ` [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver Viresh Kumar @ 2012-11-23 9:41 ` Grant Likely 2012-11-23 17:33 ` Viresh Kumar 2012-11-23 10:03 ` Lee Jones 1 sibling, 1 reply; 22+ messages in thread From: Grant Likely @ 2012-11-23 9:41 UTC (permalink / raw) To: Viresh Kumar, sameo Cc: Viresh Kumar, devicetree-discuss, spear-devel, linux-kernel, lee.jones On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote: > From: Vipul Kumar Samar <vipulkumar.samar@st.com> > > This patch extends existing DT support for stmpe devices. This updates: > - DT support from stmpe SPI and I2C drivers > - missing header files in stmpe.c > - stmpe_of_probe() with pwm, rotator and new bindings. > - Bindings are updated in binding document. > > Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V2->V3: > ------ > - Removed sub-modules DT bindings from this patch > - Retain original work done by Lee Jones > > Documentation/devicetree/bindings/mfd/stmpe.txt | 12 ++++++++---- > drivers/mfd/stmpe-i2c.c | 15 +++++++++++++++ > drivers/mfd/stmpe-spi.c | 15 +++++++++++++++ > drivers/mfd/stmpe.c | 21 +++++++++++++++------ > 4 files changed, 53 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt > index 8f0aeda..8f65c8d 100644 > --- a/Documentation/devicetree/bindings/mfd/stmpe.txt > +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt > @@ -1,13 +1,17 @@ > -* STMPE Multi-Functional Device > +* ST Microelectronics STMPE Multi-Functional Device > + > +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad, > +touchscreen, adc, pwm, rotator. > > Required properties: > - - compatible : "st,stmpe[811|1601|2401|2403]" > - - reg : I2C address of the device > + - compatible : "st,stmpe[610|801|811|1601|2401|2403]" > + - reg : I2C/SPI address of the device > > Optional properties: > - interrupts : The interrupt outputs from the controller > - - interrupt-controller : Marks the device node as an interrupt controller > - interrupt-parent : Specifies which IRQ controller we're connected to > + - irq-trigger : IRQ trigger to use for the interrupt to the host > + - irq-invert-polarity : bool, IRQ line is connected with reversed polarity This looks odd. Normally the interrupt polarity should be encoded in the irq specifier flags field. g. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-23 9:41 ` Grant Likely @ 2012-11-23 17:33 ` Viresh Kumar 2012-11-26 11:18 ` Lee Jones 2012-11-26 18:40 ` Grant Likely 0 siblings, 2 replies; 22+ messages in thread From: Viresh Kumar @ 2012-11-23 17:33 UTC (permalink / raw) To: lee.jones, Grant Likely Cc: sameo, devicetree-discuss, spear-devel, linux-kernel On 23 November 2012 15:11, Grant Likely <grant.likely@secretlab.ca> wrote: > On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> + - irq-trigger : IRQ trigger to use for the interrupt to the host >> + - irq-invert-polarity : bool, IRQ line is connected with reversed polarity > > This looks odd. Normally the interrupt polarity should be encoded in the irq > specifier flags field. Hi Grant and Lee Jones, This looks odd because stmpe is odd, i am taking the discussion held with Lee jones to this thread. So, how interrupt stuff works currently in DT.. We have a interrupt controller IC: ic: interrupt-controller@40008000 { compatible = "foo"; interrupt-controller; #interrupt-cells = <2>; ... }; And we have a user of this IC: foo-peripheral@40048000 { compatible = "foo-peripheral"; interrupt-parent = <&ic>; interrupts = <39 4>; }; Here first field of "interrupts" gives interrupt line number and the second one gives polarity, interrupt type etc.. All is good till now. Now, every interrupt controller supports the first field, but the second one depends on its capabilities. An interrupt controller might not have registers to configure interrupt polarity, type, etc of the interrupt it will service and so the second field wouldn't be available for them. For now just think stmpe is not a MFD and not a interrupt controller either. It is just a simple device, dev-foo. It will declare values of its interrupts field based on the type of interrupt controller that will service its interrupt and that can be anything like VIC/GIC/GPIO controller. Obviously nobody else than the parent IC driver can parse interrupts field of dev-foo, because only that driver understands the real meaning of these fields. Now, stmpe has a special property. It can decide the way its output interrupt line will work. i.e. its polarity and interrupt type - edle/level, etc.. This is not commonly seen in any peripheral. Now my original bindings and the real question here is about passing this information to stmpe driver. I can't pass it in interrupts field of stmpe node, as that field belongs to parent interrupt controller of stmpe. I can't pass that from child nodes of stmpe, as we are programming the interrupt coming out of stmpe and not the interrupt coming out of stmpe-gpio or stmpe-keypad. And that's why i added these bindings. Please suggest me if i am still missing something. -- viresh ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-23 17:33 ` Viresh Kumar @ 2012-11-26 11:18 ` Lee Jones 2012-11-26 18:40 ` Grant Likely 1 sibling, 0 replies; 22+ messages in thread From: Lee Jones @ 2012-11-26 11:18 UTC (permalink / raw) To: Viresh Kumar Cc: Grant Likely, sameo, devicetree-discuss, spear-devel, linux-kernel On Fri, 23 Nov 2012, Viresh Kumar wrote: > On 23 November 2012 15:11, Grant Likely <grant.likely@secretlab.ca> wrote: > > On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > >> + - irq-trigger : IRQ trigger to use for the interrupt to the host > >> + - irq-invert-polarity : bool, IRQ line is connected with reversed polarity > > > > This looks odd. Normally the interrupt polarity should be encoded in the irq > > specifier flags field. > > Hi Grant and Lee Jones, > > This looks odd because stmpe is odd, i am taking the discussion held > with Lee jones to this thread. STMPE isn't odd or special in any way. :) > So, how interrupt stuff works currently in DT.. > We have a interrupt controller IC: > > ic: interrupt-controller@40008000 { > compatible = "foo"; > interrupt-controller; > #interrupt-cells = <2>; > ... > }; > > And we have a user of this IC: > > foo-peripheral@40048000 { > compatible = "foo-peripheral"; > interrupt-parent = <&ic>; > interrupts = <39 4>; > }; > > Here first field of "interrupts" gives interrupt line number and the second one > gives polarity, interrupt type etc.. So far so good. > All is good till now. Now, every interrupt controller supports the first > field, but the second one depends on its capabilities. An interrupt controller > might not have registers to configure interrupt polarity, type, etc of > the interrupt > it will service and so the second field wouldn't be available for them. > > For now just think stmpe is not a MFD and not a interrupt controller > either. It is > just a simple device, dev-foo. > > It will declare values of its interrupts field based on the type of > interrupt controller > that will service its interrupt and that can be anything like VIC/GIC/GPIO > controller. > > Obviously nobody else than the parent IC driver can parse interrupts field > of dev-foo, because only that driver understands the real meaning of > these fields. > > Now, stmpe has a special property. It can decide the way its output > interrupt line > will work. i.e. its polarity and interrupt type - edle/level, etc.. > This is not commonly > seen in any peripheral. Now my original bindings and the real question here is > about passing this information to stmpe driver. > > I can't pass it in interrupts field of stmpe node, as that field > belongs to parent > interrupt controller of stmpe. > > I can't pass that from child nodes of stmpe, as we are programming the interrupt > coming out of stmpe and not the interrupt coming out of stmpe-gpio or > stmpe-keypad. > > And that's why i added these bindings. Please suggest me if i am still missing > something. Look into .xlate functions. -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-23 17:33 ` Viresh Kumar 2012-11-26 11:18 ` Lee Jones @ 2012-11-26 18:40 ` Grant Likely 2012-11-27 2:40 ` Viresh Kumar 1 sibling, 1 reply; 22+ messages in thread From: Grant Likely @ 2012-11-26 18:40 UTC (permalink / raw) To: Viresh Kumar, lee.jones Cc: sameo, devicetree-discuss, spear-devel, linux-kernel On Fri, 23 Nov 2012 23:03:47 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 23 November 2012 15:11, Grant Likely <grant.likely@secretlab.ca> wrote: > > On Fri, 23 Nov 2012 00:26:20 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > >> + - irq-trigger : IRQ trigger to use for the interrupt to the host > >> + - irq-invert-polarity : bool, IRQ line is connected with reversed polarity > > > > This looks odd. Normally the interrupt polarity should be encoded in the irq > > specifier flags field. > > Hi Grant and Lee Jones, > > This looks odd because stmpe is odd, i am taking the discussion held > with Lee jones to this thread. > > So, how interrupt stuff works currently in DT.. > We have a interrupt controller IC: > > ic: interrupt-controller@40008000 { > compatible = "foo"; > interrupt-controller; > #interrupt-cells = <2>; > ... > }; > > And we have a user of this IC: > > foo-peripheral@40048000 { > compatible = "foo-peripheral"; > interrupt-parent = <&ic>; > interrupts = <39 4>; > }; > > Here first field of "interrupts" gives interrupt line number and the second one > gives polarity, interrupt type etc.. > > All is good till now. Now, every interrupt controller supports the first > field, but the second one depends on its capabilities. An interrupt controller > might not have registers to configure interrupt polarity, type, etc of > the interrupt > it will service and so the second field wouldn't be available for them. > > For now just think stmpe is not a MFD and not a interrupt controller > either. It is > just a simple device, dev-foo. > > It will declare values of its interrupts field based on the type of > interrupt controller > that will service its interrupt and that can be anything like VIC/GIC/GPIO > controller. Ah, so it is configuring the way the device emits interrupts; not how the interrupt controller processes them. Fair enough. It would actually be good to ask the interrupt controller driver what kind of interrupt signal it expects for a given interrupt line. That should also solve the problem and I think it would be more useful to other devices. Can you investigate whether or not irqd_get_trigger_type() returns the information you need? g. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-26 18:40 ` Grant Likely @ 2012-11-27 2:40 ` Viresh Kumar 2012-11-27 3:28 ` Viresh Kumar 0 siblings, 1 reply; 22+ messages in thread From: Viresh Kumar @ 2012-11-27 2:40 UTC (permalink / raw) To: Grant Likely Cc: lee.jones, sameo, devicetree-discuss, spear-devel, linux-kernel On 27 November 2012 00:10, Grant Likely <grant.likely@secretlab.ca> wrote: > Ah, so it is configuring the way the device emits interrupts; not how > the interrupt controller processes them. Fair enough. Finally i am able to convince somebody that stmpe is different :) > It would actually be good to ask the interrupt controller driver what > kind of interrupt signal it expects for a given interrupt line. That > should also solve the problem and I think it would be more useful to > other devices. Can you investigate whether or not > irqd_get_trigger_type() returns the information you need? That's a pretty cool function to use. :) Will check it out :) -- viresh ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-27 2:40 ` Viresh Kumar @ 2012-11-27 3:28 ` Viresh Kumar 2012-11-27 8:40 ` Lee Jones 0 siblings, 1 reply; 22+ messages in thread From: Viresh Kumar @ 2012-11-27 3:28 UTC (permalink / raw) To: Grant Likely Cc: lee.jones, sameo, devicetree-discuss, spear-devel, linux-kernel On 27 November 2012 08:10, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 27 November 2012 00:10, Grant Likely <grant.likely@secretlab.ca> wrote: >> It would actually be good to ask the interrupt controller driver what >> kind of interrupt signal it expects for a given interrupt line. That >> should also solve the problem and I think it would be more useful to >> other devices. Can you investigate whether or not >> irqd_get_trigger_type() returns the information you need? > > That's a pretty cool function to use. :) > > Will check it out :) I was thinking about this logic in my earlier mail, don't know what stopped me from thinking it is wrong. :( Problem is with invert polarity, which the interrupt controller is not aware of. For example, suppose interrupt controller needs Rising edge interrupt, but the board has inverted the line between stmpe and IC. So, we will get Rising high from the routine you mentioned, but we need to generate opposite of that to make it rising high. And so interrupt polarity field is still required. -- viresh ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-27 3:28 ` Viresh Kumar @ 2012-11-27 8:40 ` Lee Jones 2012-11-27 8:46 ` Viresh Kumar 0 siblings, 1 reply; 22+ messages in thread From: Lee Jones @ 2012-11-27 8:40 UTC (permalink / raw) To: Viresh Kumar Cc: Grant Likely, sameo, devicetree-discuss, spear-devel, linux-kernel On Tue, 27 Nov 2012, Viresh Kumar wrote: > On 27 November 2012 08:10, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 27 November 2012 00:10, Grant Likely <grant.likely@secretlab.ca> wrote: > >> It would actually be good to ask the interrupt controller driver what > >> kind of interrupt signal it expects for a given interrupt line. That > >> should also solve the problem and I think it would be more useful to > >> other devices. Can you investigate whether or not > >> irqd_get_trigger_type() returns the information you need? > > > > That's a pretty cool function to use. :) > > > > Will check it out :) > > I was thinking about this logic in my earlier mail, don't know what stopped me > from thinking it is wrong. :( > > Problem is with invert polarity, which the interrupt controller is not aware of. > For example, suppose interrupt controller needs Rising edge interrupt, but > the board has inverted the line between stmpe and IC. So, we will get > Rising high from the routine you mentioned, but we need to generate > opposite of that to make it rising high. Surely that would be a hardware design error/quirk? Can you give an example where this has happened? > And so interrupt polarity field is still required. -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-27 8:40 ` Lee Jones @ 2012-11-27 8:46 ` Viresh Kumar 2012-11-27 19:55 ` Rabin Vincent 0 siblings, 1 reply; 22+ messages in thread From: Viresh Kumar @ 2012-11-27 8:46 UTC (permalink / raw) To: Lee Jones, rabin.vincent, Linus Walleij Cc: Grant Likely, sameo, devicetree-discuss, spear-devel, linux-kernel On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote: > On Tue, 27 Nov 2012, Viresh Kumar wrote: >> Problem is with invert polarity, which the interrupt controller is not aware of. >> For example, suppose interrupt controller needs Rising edge interrupt, but >> the board has inverted the line between stmpe and IC. So, we will get >> Rising high from the routine you mentioned, but we need to generate >> opposite of that to make it rising high. > > Surely that would be a hardware design error/quirk? Yes. > Can you give an example where this has happened? I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin would have, that's why he added that part of code :) @Rabin/Linus: Do you remember why have you added this in stmpe driver: + if (stmpe->pdata->irq_invert_polarity) + icr ^= STMPE_ICR_LSB_HIGH; + Does somebody actually need it? -- viresh ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-27 8:46 ` Viresh Kumar @ 2012-11-27 19:55 ` Rabin Vincent 2012-11-28 2:13 ` Viresh Kumar 0 siblings, 1 reply; 22+ messages in thread From: Rabin Vincent @ 2012-11-27 19:55 UTC (permalink / raw) To: Viresh Kumar Cc: Lee Jones, Linus Walleij, Grant Likely, sameo, devicetree-discuss, spear-devel, linux-kernel, l.fu 2012/11/27 Viresh Kumar <viresh.kumar@linaro.org>: > On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote: > I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin > would have, that's why he added that part of code :) > > @Rabin/Linus: Do you remember why have you added this in stmpe driver: > > + if (stmpe->pdata->irq_invert_polarity) > + icr ^= STMPE_ICR_LSB_HIGH; > + > > Does somebody actually need it? It was (as irq_rev_pol) part of Luotao Fu's proposed STMPE811 patchset (https://patchwork.kernel.org/patch/106173/) which I integrated into my version of the STMPE driver, which didn't have it in its initial version (https://patchwork.kernel.org/patch/103273/). It's not something _I_ ever used. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-27 19:55 ` Rabin Vincent @ 2012-11-28 2:13 ` Viresh Kumar 2012-11-28 9:00 ` Lee Jones 0 siblings, 1 reply; 22+ messages in thread From: Viresh Kumar @ 2012-11-28 2:13 UTC (permalink / raw) To: Rabin Vincent Cc: Lee Jones, Linus Walleij, Grant Likely, sameo, devicetree-discuss, spear-devel, linux-kernel, l.fu On 28 November 2012 01:25, Rabin Vincent <rabin@rab.in> wrote: > 2012/11/27 Viresh Kumar <viresh.kumar@linaro.org>: >> On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote: >> I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin >> would have, that's why he added that part of code :) >> >> @Rabin/Linus: Do you remember why have you added this in stmpe driver: >> >> + if (stmpe->pdata->irq_invert_polarity) >> + icr ^= STMPE_ICR_LSB_HIGH; >> + >> >> Does somebody actually need it? > > It was (as irq_rev_pol) part of Luotao Fu's proposed STMPE811 patchset > (https://patchwork.kernel.org/patch/106173/) which I integrated into my > version of the STMPE driver, which didn't have it in its initial version > (https://patchwork.kernel.org/patch/103273/). > > It's not something _I_ ever used. I grep'd kernel and nobody is using it there, so lets get rid of it completely :) I will patch it. -- viresh ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-28 2:13 ` Viresh Kumar @ 2012-11-28 9:00 ` Lee Jones 0 siblings, 0 replies; 22+ messages in thread From: Lee Jones @ 2012-11-28 9:00 UTC (permalink / raw) To: Viresh Kumar Cc: Rabin Vincent, Linus Walleij, Grant Likely, sameo, devicetree-discuss, spear-devel, linux-kernel, l.fu On Wed, 28 Nov 2012, Viresh Kumar wrote: > On 28 November 2012 01:25, Rabin Vincent <rabin@rab.in> wrote: > > 2012/11/27 Viresh Kumar <viresh.kumar@linaro.org>: > >> On 27 November 2012 14:10, Lee Jones <lee.jones@linaro.org> wrote: > >> I haven't seen this in any of SPEAr boards i have worked on. Maybe Rabin > >> would have, that's why he added that part of code :) > >> > >> @Rabin/Linus: Do you remember why have you added this in stmpe driver: > >> > >> + if (stmpe->pdata->irq_invert_polarity) > >> + icr ^= STMPE_ICR_LSB_HIGH; > >> + > >> > >> Does somebody actually need it? > > > > It was (as irq_rev_pol) part of Luotao Fu's proposed STMPE811 patchset > > (https://patchwork.kernel.org/patch/106173/) which I integrated into my > > version of the STMPE driver, which didn't have it in its initial version > > (https://patchwork.kernel.org/patch/103273/). > > > > It's not something _I_ ever used. > > I grep'd kernel and nobody is using it there, so lets get rid of it > completely :) > I will patch it. Great. :) -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver 2012-11-22 18:56 ` [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver Viresh Kumar 2012-11-23 9:41 ` Grant Likely @ 2012-11-23 10:03 ` Lee Jones 1 sibling, 0 replies; 22+ messages in thread From: Lee Jones @ 2012-11-23 10:03 UTC (permalink / raw) To: Viresh Kumar Cc: sameo, devicetree-discuss, linux-kernel, spear-devel, Vipul Kumar Samar On Fri, 23 Nov 2012, Viresh Kumar wrote: > From: Vipul Kumar Samar <vipulkumar.samar@st.com> > > This patch extends existing DT support for stmpe devices. This updates: > - DT support from stmpe SPI and I2C drivers > - missing header files in stmpe.c > - stmpe_of_probe() with pwm, rotator and new bindings. > - Bindings are updated in binding document. > > Signed-off-by: Vipul Kumar Samar <vipulkumar.samar@st.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > V2->V3: > ------ > - Removed sub-modules DT bindings from this patch > - Retain original work done by Lee Jones > > Documentation/devicetree/bindings/mfd/stmpe.txt | 12 ++++++++---- > drivers/mfd/stmpe-i2c.c | 15 +++++++++++++++ > drivers/mfd/stmpe-spi.c | 15 +++++++++++++++ > drivers/mfd/stmpe.c | 21 +++++++++++++++------ > 4 files changed, 53 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt > index 8f0aeda..8f65c8d 100644 > --- a/Documentation/devicetree/bindings/mfd/stmpe.txt > +++ b/Documentation/devicetree/bindings/mfd/stmpe.txt > @@ -1,13 +1,17 @@ > -* STMPE Multi-Functional Device > +* ST Microelectronics STMPE Multi-Functional Device > + > +STMPE is an MFD device which may expose following inbuilt devices: gpio, keypad, > +touchscreen, adc, pwm, rotator. > > Required properties: > - - compatible : "st,stmpe[811|1601|2401|2403]" > - - reg : I2C address of the device > + - compatible : "st,stmpe[610|801|811|1601|2401|2403]" > + - reg : I2C/SPI address of the device > > Optional properties: > - interrupts : The interrupt outputs from the controller > - - interrupt-controller : Marks the device node as an interrupt controller On closer inspection, this is actually incorrect. The STMPE _is_ an interrupt-controller. > - interrupt-parent : Specifies which IRQ controller we're connected to > + - irq-trigger : IRQ trigger to use for the interrupt to the host > + - irq-invert-polarity : bool, IRQ line is connected with reversed polarity I've told you about this already. ;) > - i2c-client-wake : Marks the input device as wakable > - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024 > > diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c > index c734dc3..003c49b 100644 > --- a/drivers/mfd/stmpe-i2c.c > +++ b/drivers/mfd/stmpe-i2c.c > @@ -13,6 +13,7 @@ > #include <linux/interrupt.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/types.h> > #include "stmpe.h" > > @@ -81,6 +82,19 @@ static const struct i2c_device_id stmpe_i2c_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, stmpe_id); > > +#ifdef CONFIG_OF > +static const struct of_device_id stmpe_dt_ids[] = { > + { .compatible = "st,stmpe610", .data = &stmpe_i2c_id[0], }, > + { .compatible = "st,stmpe801", .data = &stmpe_i2c_id[1], }, > + { .compatible = "st,stmpe811", .data = &stmpe_i2c_id[2], }, > + { .compatible = "st,stmpe1601", .data = &stmpe_i2c_id[3], }, > + { .compatible = "st,stmpe2401", .data = &stmpe_i2c_id[4], }, > + { .compatible = "st,stmpe2403", .data = &stmpe_i2c_id[5], }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, stmpe_dt_ids); > +#endif > + > static struct i2c_driver stmpe_i2c_driver = { > .driver = { > .name = "stmpe-i2c", > @@ -88,6 +102,7 @@ static struct i2c_driver stmpe_i2c_driver = { > #ifdef CONFIG_PM > .pm = &stmpe_dev_pm_ops, > #endif > + .of_match_table = of_match_ptr(stmpe_dt_ids), > }, > .probe = stmpe_i2c_probe, > .remove = __devexit_p(stmpe_i2c_remove), > diff --git a/drivers/mfd/stmpe-spi.c b/drivers/mfd/stmpe-spi.c > index f86e3fc..e482f5f 100644 > --- a/drivers/mfd/stmpe-spi.c > +++ b/drivers/mfd/stmpe-spi.c > @@ -10,6 +10,7 @@ > #include <linux/interrupt.h> > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/spi/spi.h> > #include <linux/types.h> > #include "stmpe.h" > @@ -119,6 +120,19 @@ static const struct spi_device_id stmpe_spi_id[] = { > }; > MODULE_DEVICE_TABLE(spi, stmpe_id); > > +#ifdef CONFIG_OF > +static const struct of_device_id stmpe_dt_ids[] = { > + { .compatible = "st,stmpe610", .data = (void *)STMPE610, }, > + { .compatible = "st,stmpe801", .data = (void *)STMPE801, }, > + { .compatible = "st,stmpe811", .data = (void *)STMPE811, }, > + { .compatible = "st,stmpe1601", .data = (void *)STMPE1601, }, > + { .compatible = "st,stmpe2401", .data = (void *)STMPE2401, }, > + { .compatible = "st,stmpe2403", .data = (void *)STMPE2403, }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, stmpe_dt_ids); > +#endif > + > static struct spi_driver stmpe_spi_driver = { > .driver = { > .name = "stmpe-spi", > @@ -126,6 +140,7 @@ static struct spi_driver stmpe_spi_driver = { > #ifdef CONFIG_PM > .pm = &stmpe_dev_pm_ops, > #endif > + .of_match_table = of_match_ptr(stmpe_dt_ids), > }, > .probe = stmpe_spi_probe, > .remove = __devexit_p(stmpe_spi_remove), > diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c > index 6086481..e2c0dda 100644 > --- a/drivers/mfd/stmpe.c > +++ b/drivers/mfd/stmpe.c > @@ -7,6 +7,7 @@ > * Author: Rabin Vincent <rabin.vincent@stericsson.com> for ST-Ericsson > */ > > +#include <linux/err.h> > #include <linux/export.h> > #include <linux/gpio.h> > #include <linux/interrupt.h> > @@ -14,6 +15,8 @@ > #include <linux/irqdomain.h> > #include <linux/kernel.h> > #include <linux/mfd/core.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > #include <linux/pm.h> > #include <linux/slab.h> > #include "stmpe.h" > @@ -1028,18 +1031,24 @@ void __devinit stmpe_of_probe(struct stmpe_platform_data *pdata, > > pdata->autosleep = (pdata->autosleep_timeout) ? true : false; > > + of_property_read_u32(np, "irq-trigger", &pdata->irq_trigger); > + > + if (of_property_read_bool(np, "irq-invert-polarity")) > + pdata->irq_invert_polarity = true; > + You don't need these. > for_each_child_of_node(np, child) { > if (!strcmp(child->name, "stmpe_gpio")) { > pdata->blocks |= STMPE_BLOCK_GPIO; > - } > - if (!strcmp(child->name, "stmpe_keypad")) { > + } else if (!strcmp(child->name, "stmpe_keypad")) { > pdata->blocks |= STMPE_BLOCK_KEYPAD; > - } > - if (!strcmp(child->name, "stmpe_touchscreen")) { > + } else if (!strcmp(child->name, "stmpe_touchscreen")) { > pdata->blocks |= STMPE_BLOCK_TOUCHSCREEN; > - } > - if (!strcmp(child->name, "stmpe_adc")) { > + } else if (!strcmp(child->name, "stmpe_adc")) { > pdata->blocks |= STMPE_BLOCK_ADC; > + } else if (!strcmp(child->name, "stmpe_pwm")) { > + pdata->blocks |= STMPE_BLOCK_PWM; > + } else if (!strcmp(child->name, "stmpe_rotator")) { > + pdata->blocks |= STMPE_BLOCK_ROTATOR; > } > } > } > -- > 1.7.12.rc2.18.g61b472e > -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order 2012-11-22 18:56 [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Viresh Kumar 2012-11-22 18:56 ` [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver Viresh Kumar 2012-11-22 18:56 ` [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver Viresh Kumar @ 2012-11-26 11:16 ` Samuel Ortiz 2012-11-26 11:22 ` Viresh Kumar 2 siblings, 1 reply; 22+ messages in thread From: Samuel Ortiz @ 2012-11-26 11:16 UTC (permalink / raw) To: Viresh Kumar; +Cc: devicetree-discuss, linux-kernel, spear-devel, lee.jones Hi Viresh, On Fri, Nov 23, 2012 at 12:26:18AM +0530, Viresh Kumar wrote: > This helps managing them better and also reduces chances of adding an header > file twice. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/mfd/stmpe-spi.c | 2 +- > drivers/mfd/stmpe.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) I fail to see the point of this patch, sorry. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order 2012-11-26 11:16 ` [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Samuel Ortiz @ 2012-11-26 11:22 ` Viresh Kumar 2012-11-26 13:25 ` Lee Jones 0 siblings, 1 reply; 22+ messages in thread From: Viresh Kumar @ 2012-11-26 11:22 UTC (permalink / raw) To: Samuel Ortiz; +Cc: devicetree-discuss, linux-kernel, spear-devel, lee.jones On 26 November 2012 16:46, Samuel Ortiz <sameo@linux.intel.com> wrote: > Hi Viresh, > > On Fri, Nov 23, 2012 at 12:26:18AM +0530, Viresh Kumar wrote: >> This helps managing them better and also reduces chances of adding an header >> file twice. The aim is to maintain the list of header files in alphabetical order. It helps in maintaining them.. I was adding some header files for my 3rd patch and was looking for the best place to add them and because the list wasn't sorted, i sorted it out in a separate patch. -- viresh ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order 2012-11-26 11:22 ` Viresh Kumar @ 2012-11-26 13:25 ` Lee Jones 2012-11-26 14:55 ` Viresh Kumar 0 siblings, 1 reply; 22+ messages in thread From: Lee Jones @ 2012-11-26 13:25 UTC (permalink / raw) To: Viresh Kumar; +Cc: Samuel Ortiz, devicetree-discuss, linux-kernel, spear-devel On Mon, 26 Nov 2012, Viresh Kumar wrote: > On 26 November 2012 16:46, Samuel Ortiz <sameo@linux.intel.com> wrote: > > Hi Viresh, > > > > On Fri, Nov 23, 2012 at 12:26:18AM +0530, Viresh Kumar wrote: > >> This helps managing them better and also reduces chances of adding an header > >> file twice. > > The aim is to maintain the list of header files in alphabetical order. > It helps in maintaining > them.. I was adding some header files for my 3rd patch and was looking > for the best > place to add them and because the list wasn't sorted, i sorted it out > in a separate patch. Why do you need to sort them? Is there _really_ a need? -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order 2012-11-26 13:25 ` Lee Jones @ 2012-11-26 14:55 ` Viresh Kumar 2012-11-26 15:31 ` Lee Jones 0 siblings, 1 reply; 22+ messages in thread From: Viresh Kumar @ 2012-11-26 14:55 UTC (permalink / raw) To: Lee Jones; +Cc: Samuel Ortiz, devicetree-discuss, linux-kernel, spear-devel On 26 November 2012 18:55, Lee Jones <lee.jones@linaro.org> wrote: > Why do you need to sort them? Is there _really_ a need? Many people & maintainers like to have their header files ordered. The reason for that is: If they are not ordered, there is a possibility of adding an header file multiple times in a file. This might not do something serious when thinking about compilation time or Image size, but adding an header file multiple times is simply wrong. This can't be caught in patch reviews most of the time, as diff may not show the earlier inclusion. That's why i ordered them. And i don't see any harm in doing so. -- viresh ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order 2012-11-26 14:55 ` Viresh Kumar @ 2012-11-26 15:31 ` Lee Jones 2012-11-26 16:05 ` Samuel Ortiz 0 siblings, 1 reply; 22+ messages in thread From: Lee Jones @ 2012-11-26 15:31 UTC (permalink / raw) To: Viresh Kumar; +Cc: Samuel Ortiz, devicetree-discuss, linux-kernel, spear-devel On Mon, 26 Nov 2012, Viresh Kumar wrote: > On 26 November 2012 18:55, Lee Jones <lee.jones@linaro.org> wrote: > > > Why do you need to sort them? Is there _really_ a need? > > Many people & maintainers like to have their header files ordered. The > reason for that is: > > If they are not ordered, there is a possibility of adding an header file > multiple times in a file. This might not do something serious > when thinking about compilation time or Image size, but adding an header > file multiple times is simply wrong. > > This can't be caught in patch reviews most of the time, as diff may not show > the earlier inclusion. > > That's why i ordered them. And i don't see any harm in doing so. I don't see any harm in it, but it's pretty pointless. There aren't many maintainers who insist on such things, and the ones that do normally carry out these OCD actions themselves. :) -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order 2012-11-26 15:31 ` Lee Jones @ 2012-11-26 16:05 ` Samuel Ortiz 0 siblings, 0 replies; 22+ messages in thread From: Samuel Ortiz @ 2012-11-26 16:05 UTC (permalink / raw) To: Lee Jones; +Cc: Viresh Kumar, devicetree-discuss, linux-kernel, spear-devel On Mon, Nov 26, 2012 at 03:31:45PM +0000, Lee Jones wrote: > On Mon, 26 Nov 2012, Viresh Kumar wrote: > > > On 26 November 2012 18:55, Lee Jones <lee.jones@linaro.org> wrote: > > > > > Why do you need to sort them? Is there _really_ a need? > > > > Many people & maintainers like to have their header files ordered. The > > reason for that is: > > > > If they are not ordered, there is a possibility of adding an header file > > multiple times in a file. This might not do something serious > > when thinking about compilation time or Image size, but adding an header > > file multiple times is simply wrong. > > > > This can't be caught in patch reviews most of the time, as diff may not show > > the earlier inclusion. > > > > That's why i ordered them. And i don't see any harm in doing so. > > I don't see any harm in it, but it's pretty pointless. > > There aren't many maintainers who insist on such things, And I'm not one of those. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-11-28 9:00 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-22 18:56 [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Viresh Kumar 2012-11-22 18:56 ` [PATCH V1 2/3] mfd: stmpe-i2c: Move .driver structure fields inside {} in stmpe_i2c_driver Viresh Kumar 2012-11-26 11:15 ` Samuel Ortiz 2012-11-22 18:56 ` [PATCH V3 3/3] mfd: stmpe: Update DT support in stmpe driver Viresh Kumar 2012-11-23 9:41 ` Grant Likely 2012-11-23 17:33 ` Viresh Kumar 2012-11-26 11:18 ` Lee Jones 2012-11-26 18:40 ` Grant Likely 2012-11-27 2:40 ` Viresh Kumar 2012-11-27 3:28 ` Viresh Kumar 2012-11-27 8:40 ` Lee Jones 2012-11-27 8:46 ` Viresh Kumar 2012-11-27 19:55 ` Rabin Vincent 2012-11-28 2:13 ` Viresh Kumar 2012-11-28 9:00 ` Lee Jones 2012-11-23 10:03 ` Lee Jones 2012-11-26 11:16 ` [PATCH V1 1/3] mfd: stmpe: Arrange #include <header files> in alphabetical order Samuel Ortiz 2012-11-26 11:22 ` Viresh Kumar 2012-11-26 13:25 ` Lee Jones 2012-11-26 14:55 ` Viresh Kumar 2012-11-26 15:31 ` Lee Jones 2012-11-26 16:05 ` Samuel Ortiz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).