| Message ID | 1482182934-18559-3-git-send-email-lilja.magnus@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>Вторник, 20 декабря 2016, 0:28 +03:00 от Magnus Lilja <lilja.magnus@gmail.com>: > >All supported mc13xxx devices have active high interrupt outputs. Make sure >to configure the interrupt as active high by passing the IRQF_TRIGGER_HIGH >flag. This is required at least on the i.MX31 PDK board. > >Signed-off-by: Magnus Lilja < lilja.magnus@gmail.com > >--- > > drivers/mfd/mc13xxx-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c >index d7f54e4..4cbe6b7 100644 >--- a/drivers/mfd/mc13xxx-core.c >+++ b/drivers/mfd/mc13xxx-core.c >@@ -440,7 +440,7 @@ int mc13xxx_common_init(struct device *dev) > mc13xxx->irq_chip.irqs = mc13xxx->irqs; > mc13xxx->irq_chip.num_irqs = ARRAY_SIZE(mc13xxx->irqs); > >-ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT, >+ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT | IRQF_TRIGGER_HIGH, > 0, &mc13xxx->irq_chip, &mc13xxx->irq_data); > if (ret) > return ret; IRQ line can be passed through inverter to IC. On my opinion the best way to handle all possible situations is parse devicetree IRQ flags and pass to the driver. ---
On 20 December 2016 at 06:20, Alexander Shiyan <shc_work@mail.ru> wrote: >>Вторник, 20 декабря 2016, 0:28 +03:00 от Magnus Lilja <lilja.magnus@gmail.com>: >> >>All supported mc13xxx devices have active high interrupt outputs. Make sure >>to configure the interrupt as active high by passing the IRQF_TRIGGER_HIGH >>flag. This is required at least on the i.MX31 PDK board. >> >>Signed-off-by: Magnus Lilja < lilja.magnus@gmail.com > >>--- >> >> drivers/mfd/mc13xxx-core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >>diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c >>index d7f54e4..4cbe6b7 100644 >>--- a/drivers/mfd/mc13xxx-core.c >>+++ b/drivers/mfd/mc13xxx-core.c >>@@ -440,7 +440,7 @@ int mc13xxx_common_init(struct device *dev) >> mc13xxx->irq_chip.irqs = mc13xxx->irqs; >> mc13xxx->irq_chip.num_irqs = ARRAY_SIZE(mc13xxx->irqs); >> >>-ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT, >>+ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT | IRQF_TRIGGER_HIGH, >> 0, &mc13xxx->irq_chip, &mc13xxx->irq_data); >> if (ret) >> return ret; > > IRQ line can be passed through inverter to IC. > On my opinion the best way to handle all possible situations is parse > devicetree IRQ flags and pass to the driver. My guess is that most boards follow the evaluation boards/kits and don't have an inverter so I think the default should be TRIG_HIGH since that will make most boards work automatically. But I can have a look at making it configurable (with and without the use of device tree), but for the device tree stuff I would need pointers to similar code since my experience with that is none. Any pointers to similar code? Regards. Magnus
>Вторник, 20 декабря 2016, 22:35 +03:00 от Magnus Lilja <lilja.magnus@gmail.com>: > >On 20 December 2016 at 06:20, Alexander Shiyan < shc_work@mail.ru > wrote: >>>Вторник, 20 декабря 2016, 0:28 +03:00 от Magnus Lilja < lilja.magnus@gmail.com >: >>> >>>All supported mc13xxx devices have active high interrupt outputs. Make sure >>>to configure the interrupt as active high by passing the IRQF_TRIGGER_HIGH >>>flag. This is required at least on the i.MX31 PDK board. >>> >>>Signed-off-by: Magnus Lilja < lilja.magnus@gmail.com > >>>--- >>> >>> drivers/mfd/mc13xxx-core.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>>diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c >>>index d7f54e4..4cbe6b7 100644 >>>--- a/drivers/mfd/mc13xxx-core.c >>>+++ b/drivers/mfd/mc13xxx-core.c >>>@@ -440,7 +440,7 @@ int mc13xxx_common_init(struct device *dev) >>> mc13xxx->irq_chip.irqs = mc13xxx->irqs; >>> mc13xxx->irq_chip.num_irqs = ARRAY_SIZE(mc13xxx->irqs); >>> >>>-ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT, >>>+ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT | IRQF_TRIGGER_HIGH, >>> 0, &mc13xxx->irq_chip, &mc13xxx->irq_data); >>> if (ret) >>> return ret; >> >> IRQ line can be passed through inverter to IC. >> On my opinion the best way to handle all possible situations is parse >> devicetree IRQ flags and pass to the driver. > >My guess is that most boards follow the evaluation boards/kits and >don't have an inverter so I think the default should be TRIG_HIGH >since that will make most boards work automatically. But I can have a >look at making it configurable (with and without the use of device >tree), but for the device tree stuff I would need pointers to similar >code since my experience with that is none. >Any pointers to similar code? Hello. Perhaps I'm wrong and the desired active level has setup at the IRQ code level. Otherwise, I think you need to use something like the following: unsigned flags = irqd_get_trigger_type(irq_get_irq_data(irq)); flags = (flags == IRQ_TYPE_NONE) ? IRQF_TRIGGER_HIGH : flags; ret = regmap_add_irq_chip(..., IRQF_ONESHOT | flags, ...); Please fix me. Thanks. ---
Hi, On 21 December 2016 at 07:00, Alexander Shiyan <shc_work@mail.ru> wrote: >>Вторник, 20 декабря 2016, 22:35 +03:00 от Magnus Lilja <lilja.magnus@gmail.com>: >> >>On 20 December 2016 at 06:20, Alexander Shiyan < shc_work@mail.ru > wrote: >>>>Вторник, 20 декабря 2016, 0:28 +03:00 от Magnus Lilja < lilja.magnus@gmail.com >: >>>> >>>>All supported mc13xxx devices have active high interrupt outputs. Make sure >>>>to configure the interrupt as active high by passing the IRQF_TRIGGER_HIGH >>>>flag. This is required at least on the i.MX31 PDK board. >>>> >>>>Signed-off-by: Magnus Lilja < lilja.magnus@gmail.com > >>>>--- >>>> >>>> drivers/mfd/mc13xxx-core.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>>diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c >>>>index d7f54e4..4cbe6b7 100644 >>>>--- a/drivers/mfd/mc13xxx-core.c >>>>+++ b/drivers/mfd/mc13xxx-core.c >>>>@@ -440,7 +440,7 @@ int mc13xxx_common_init(struct device *dev) >>>> mc13xxx->irq_chip.irqs = mc13xxx->irqs; >>>> mc13xxx->irq_chip.num_irqs = ARRAY_SIZE(mc13xxx->irqs); >>>> >>>>-ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT, >>>>+ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT | IRQF_TRIGGER_HIGH, >>>> 0, &mc13xxx->irq_chip, &mc13xxx->irq_data); >>>> if (ret) >>>> return ret; >>> >>> IRQ line can be passed through inverter to IC. >>> On my opinion the best way to handle all possible situations is parse >>> devicetree IRQ flags and pass to the driver. >> >>My guess is that most boards follow the evaluation boards/kits and >>don't have an inverter so I think the default should be TRIG_HIGH >>since that will make most boards work automatically. But I can have a >>look at making it configurable (with and without the use of device >>tree), but for the device tree stuff I would need pointers to similar >>code since my experience with that is none. >>Any pointers to similar code? > > Hello. > > Perhaps I'm wrong and the desired active level has setup at the IRQ code level. Don't think I understand what you mean here. > Otherwise, I think you need to use something like the following: > > unsigned flags = irqd_get_trigger_type(irq_get_irq_data(irq)); > flags = (flags == IRQ_TYPE_NONE) ? IRQF_TRIGGER_HIGH : flags; > ret = regmap_add_irq_chip(..., IRQF_ONESHOT | flags, ...); That would work but I don't know how one would set the trigger type in case of not using device trees. But that would only be a problem if a board really has an inverter on the IRQ line so that can be solved later, and might be not necessary to solve if mx31 boards are converted to device tree usage. I guess that get trigger_type can be set via the device tree magic when using device trees. Regards, Magnus
Hi Magnus, On Mon, Dec 19, 2016 at 7:28 PM, Magnus Lilja <lilja.magnus@gmail.com> wrote: > All supported mc13xxx devices have active high interrupt outputs. Make sure > to configure the interrupt as active high by passing the IRQF_TRIGGER_HIGH > flag. This is required at least on the i.MX31 PDK board. > > Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com> What is the commit that caused the breakage in the driver? Please specify in the commit log, add a Fixes tag and Cc: stable like mentioned on patch 1/2.
Hi On 21 December 2016 at 21:10, Fabio Estevam <festevam@gmail.com> wrote: > Hi Magnus, > > On Mon, Dec 19, 2016 at 7:28 PM, Magnus Lilja <lilja.magnus@gmail.com> wrote: >> All supported mc13xxx devices have active high interrupt outputs. Make sure >> to configure the interrupt as active high by passing the IRQF_TRIGGER_HIGH >> flag. This is required at least on the i.MX31 PDK board. >> >> Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com> > > What is the commit that caused the breakage in the driver? Please > specify in the commit log, add a Fixes tag and Cc: stable like > mentioned on patch 1/2. Will add that info and cc: stable. And while investigating which commit that caused this I noticed that the prior to that commit IRQF_TRIGGER_HIGH was always passed without possibility to change so I wonder if it really is necessary to support any inverters in the IRQ line.. Thanks, Magnus
On Wed, Dec 21, 2016 at 6:24 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: > Correct, I would recommend to postpone adding any extensions to the driver > platform data, which by the way is found in include/linux/mfd/mc13xxx.h > > The extension can be added only when it becomes needed. Yes, I agree. > FWIW I ran the v4.9-rc kernel with device trees on i.MX31 Lite board > with MC13783, and what I can confirm is that in my case the proposed > change is not needed at all. Thus I'm going to verify shortly that > the commit does not break the currently correct runtime behaviour on > my board. Nice to see imx31 with dt support :-)
Hi, On 22 December 2016 at 02:08, Vladimir Zapolskiy <vz@mleia.com> wrote: > On 12/21/2016 10:28 PM, Fabio Estevam wrote: >> On Wed, Dec 21, 2016 at 6:24 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: >> >>> Correct, I would recommend to postpone adding any extensions to the driver >>> platform data, which by the way is found in include/linux/mfd/mc13xxx.h >>> >>> The extension can be added only when it becomes needed. >> >> Yes, I agree. >> > > So, for reference here is a snippet from the i.MX31 Lite board DTS: > > &spi2 { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_cspi2>; > > /* > * Note that intentionally there is no GPIO CS, i.MX31 CSPI > * controller does not support GPIO CS and this must be specially > * accounted in the driver, which currently fails to register > * without a provided "cs-gpios" property. > */ > fsl,spi-num-chipselects = <1>; > status = "okay"; > > pmic: mc13783@0 { > compatible = "fsl,mc13783"; > reg = <0>; > spi-cs-high; > spi-max-frequency = <1000000>; > interrupt-parent = <&gpio1>; > interrupts = <3 IRQ_TYPE_EDGE_RISING>; > > fsl,mc13xxx-uses-rtc; > }; > }; > > I'm running the current pre-rc1 v4.10. The MFD device is registered and > MC13783 primary interrupt connected to GPIO1_3 is fired as expected > independently if the fixup under discussion is applied or not: > > root@mx31lite:~# dmesg | grep spi > [ 2.017217] mc13xxx spi32766.0: mc13783: rev: 3.3, fin: 0, fab: 0, icid: 2/0 > [ 2.072029] spi_imx 50010000.cspi: probed > > root@mx31lite:~# dmesg | grep rtc0 > [ 2.682459] mc13xxx-rtc mc13783-rtc: rtc core: registered mc13783-rtc as rtc0 > > root@mx31lite:~# cat /proc/interrupts | grep mc13xxx-rtc > 176: 0 spi32766.0 31 Edge mc13xxx-rtc > 177: 0 spi32766.0 25 Edge mc13xxx-rtc > > root@mx31lite:~# echo +5 > /sys/class/rtc/rtc0/wakealarm ; sleep 5 > > root@mx31lite:~# cat /proc/interrupts | grep mc13xxx-rtc > 176: 0 spi32766.0 31 Edge mc13xxx-rtc > 177: 1 spi32766.0 25 Edge mc13xxx-rtc > > So the change at least does not break i.MX31 Lite board with DTS support, > however I'm not sure if the change is still valid for any board with DTS > if the type of the interrupt is specified as IRQ_TYPE_EDGE_FALLING. So, have we reached a conclusion that a solution based on the code below is good enough for now? Further device tree development would make it possible to support other triggers on i.MX31 boards. And any board that already has dt support can override TRIGGER_HIGH since irqd_get_trigger_type() will return something else than IRQ_TYPE_NONE. > unsigned flags = irqd_get_trigger_type(irq_get_irq_data(irq)); > flags = (flags == IRQ_TYPE_NONE) ? IRQF_TRIGGER_HIGH : flags; > ret = regmap_add_irq_chip(..., IRQF_ONESHOT | flags, ...); The above code works on both 3.18.x and 4.9. Regards, Magnus
On Thu, Dec 22, 2016 at 6:16 PM, Magnus Lilja <lilja.magnus@gmail.com> wrote: > So, have we reached a conclusion that a solution based on the code > below is good enough for now? Further device tree development would > make it possible to support other triggers on i.MX31 boards. And any > board that already has dt support can override TRIGGER_HIGH since > irqd_get_trigger_type() will return something else than IRQ_TYPE_NONE. I would say that you could send a v2 with the only difference being the commit log explaining which was the commit that caused the problem, add a Fixes tag and Cc: stable. Thanks
Hi Vladimir, On Wed, Dec 21, 2016 at 11:08 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: > Basic support is done, IOMUX driver is developed, practically everything > but multimedia and USB are tested and working well, SPI driver needs > some minor special attention mentioned above, hopefully I'll find time > to submit core imx31.dtsi changes this week. Do you plan to submit the mx31 pinctrl driver? It would be nice if we could move mx31 to DT. Thanks
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c index d7f54e4..4cbe6b7 100644 --- a/drivers/mfd/mc13xxx-core.c +++ b/drivers/mfd/mc13xxx-core.c @@ -440,7 +440,7 @@ int mc13xxx_common_init(struct device *dev) mc13xxx->irq_chip.irqs = mc13xxx->irqs; mc13xxx->irq_chip.num_irqs = ARRAY_SIZE(mc13xxx->irqs); - ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT, + ret = regmap_add_irq_chip(mc13xxx->regmap, mc13xxx->irq, IRQF_ONESHOT | IRQF_TRIGGER_HIGH, 0, &mc13xxx->irq_chip, &mc13xxx->irq_data); if (ret) return ret;
All supported mc13xxx devices have active high interrupt outputs. Make sure to configure the interrupt as active high by passing the IRQF_TRIGGER_HIGH flag. This is required at least on the i.MX31 PDK board. Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com> --- drivers/mfd/mc13xxx-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)