[PATCH 0/2] Patches to make i.MX31 PDK board boot again

[PATCH 0/2] Patches to make i.MX31 PDK board boot again

[Magnus Lilja]

Hi,

I've been working on getting the current kernel (v4.9) as well as the stable
kernels to boot on the i.MX31 PDK board. The following is my summary of 
which patches/changes are required for each kernel series. Some patches
have been posted on the mailing lists, one is in the v4.9 kernel and then
there are two new patches in this series.

On all kernels the standard imx_v6_v7_defconfig was used.

The patches in this series applies on v4.9 as well as the imx-soc/for-next
tree as of today.

Kernel 4.9:
[RESERVE-PATCH] [IMX-WDT-PATCH] [IPU-IRQ-PATCH] [MC13xxx-PATCH]

Kernel 4.8.x:
[RESERVE-PATCH] [IMX-WDT-PATCH] [IPU-IRQ-PATCH] [MC13xxx-PATCH] 
[IMX-SPI-PATCH]

Kernel 4.4.x:
[RESERVE-PATCH] [IMX-WDT-PATCH] [IPU-IRQ-PATCH] [MC13xxx-PATCH]

Kernel 4.1.x:
[RESERVE-PATCH] [IMX-WDT-PATCH] [IPU-IRQ-PATCH] [MC13xxx-PATCH]

Kernel 3.18.x:
[RESERVE-PATCH] [MC13xxx-PATCH]

Kernel 3.16.x:
[RESERVE-PATCH]

Kernel 3.14.x, 3.12.x, 3.10..x, 3.4.x, 3.2.x:
No changes needed.

[RESERVE-PATCH] https://marc.info/?l=linux-arm-kernel&m=148168166012794&w=2
[IMX-WDT-PATCH] https://marc.info/?l=linux-arm-kernel&m=148141853206440&w=2
[IPU-IRQ-PATCH] Patch 1 in my series
[MC13xxx-PATCH] Patch 2 in my series
[IMX-SPI-PATCH] Commit 2636ba8fa39915c7b8d73166961ebbb4c14251cd

Regards, Magnus

Magnus Lilja (2):
  i.MX31: ipu: Make sure the interrupt routine checks all interrupts.
  mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

 drivers/dma/ipu/ipu_irq.c  | 2 +-
 drivers/mfd/mc13xxx-core.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH 1/2] i.MX31: ipu: Make sure the interrupt routine checks all interrupts.

[Magnus Lilja]

Commit 3d8cc00073d6750ffe883685e49b2e4a0f596370 consolidated the two
interrupts routines into one, but the remaining interrupt routine only
checks the status of the error interrupts, not the normal interrupts.

This patch fixes that problem (tested on i.MX31 PDK board).

Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com>
---
 drivers/dma/ipu/ipu_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/ipu/ipu_irq.c b/drivers/dma/ipu/ipu_irq.c
index dd184b5..2846278 100644
--- a/drivers/dma/ipu/ipu_irq.c
+++ b/drivers/dma/ipu/ipu_irq.c
@@ -272,7 +272,7 @@ static void ipu_irq_handler(struct irq_desc *desc)
 	u32 status;
 	int i, line;
 
-	for (i = IPU_IRQ_NR_FN_BANKS; i < IPU_IRQ_NR_BANKS; i++) {
+	for (i = 0; i < IPU_IRQ_NR_BANKS; i++) {
 		struct ipu_irq_bank *bank = irq_bank + i;
 
 		raw_spin_lock(&bank_lock);
-- 
2.7.4

[PATCH 1/2] i.MX31: ipu: Make sure the interrupt routine checks all interrupts.

[Fabio Estevam]

Hi Magnus,

On Mon, Dec 19, 2016 at 7:28 PM, Magnus Lilja <lilja.magnus@gmail.com> wrote:
> Commit 3d8cc00073d6750ffe883685e49b2e4a0f596370 consolidated the two

The standard form to specify a commit is 3d8cc00073d6 ("dmaengine:
ipu: Consolidate duplicated irq handlers")

> interrupts routines into one, but the remaining interrupt routine only
> checks the status of the error interrupts, not the normal interrupts.
>
> This patch fixes that problem (tested on i.MX31 PDK board).
>
> Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com>

Patch looks good, but I have some suggestions:

- Subject should start with dmaengine, so something like:
Subject: dmaengine: ipu: Make sure the ....

- You sent these two patches on a series, but as they touch different
subsystems they could be sent separately, each one to the proper
maintainer. For dmaengine the maintainer is Vinod. Hint: you can run
./scripts/get_maintainer.pl on your patch and it will list the
maintainer and lists the patches should be sent to.

You should also add a Fixes tag above your Signed-off-by like this:
Fixes: 3d8cc00073d6 ("dmaengine: ipu: Consolidate duplicated irq handlers")
Cc: <stable@vger.kernel.org> # 4.3.x

[PATCH 1/2] i.MX31: ipu: Make sure the interrupt routine checks all interrupts.

[Magnus Lilja]

Hi Fabio,

On 21 December 2016 at 21:08, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Magnus,
>
> On Mon, Dec 19, 2016 at 7:28 PM, Magnus Lilja <lilja.magnus@gmail.com> wrote:
>> Commit 3d8cc00073d6750ffe883685e49b2e4a0f596370 consolidated the two
>
> The standard form to specify a commit is 3d8cc00073d6 ("dmaengine:
> ipu: Consolidate duplicated irq handlers")
>
>> interrupts routines into one, but the remaining interrupt routine only
>> checks the status of the error interrupts, not the normal interrupts.
>>
>> This patch fixes that problem (tested on i.MX31 PDK board).
>>
>> Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com>
>
> Patch looks good, but I have some suggestions:
>
> - Subject should start with dmaengine, so something like:
> Subject: dmaengine: ipu: Make sure the ....
>
> - You sent these two patches on a series, but as they touch different
> subsystems they could be sent separately, each one to the proper
> maintainer. For dmaengine the maintainer is Vinod. Hint: you can run
> ./scripts/get_maintainer.pl on your patch and it will list the
> maintainer and lists the patches should be sent to.
>
> You should also add a Fixes tag above your Signed-off-by like this:
> Fixes: 3d8cc00073d6 ("dmaengine: ipu: Consolidate duplicated irq handlers")
> Cc: <stable@vger.kernel.org> # 4.3.x

Thanks for review. I will add take care of your comments and send a
new standalone patch.

Regards, Magnus

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Magnus Lilja]

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;
-- 
2.7.4

Re: [PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Alexander Shiyan]

>???????, 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.

---

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Magnus Lilja]

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

Re: [PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Alexander Shiyan]

>???????, 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.

---

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Magnus Lilja]

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

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Vladimir Zapolskiy]

Hi,

On 12/21/2016 09:41 PM, Magnus Lilja wrote:
> 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.

you may look at drivers/mfd/palmas.c and include/linux/mfd/palmas.h
to get an idea how irq flags are passed through the platform data in
a similar case.

> But that would only be a problem if a board really has an inverter on
> the IRQ line so that can be solved later,

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.

> 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.
> 

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.

--
With best wishes,
Vladimir

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Fabio Estevam]

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 :-)

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Vladimir Zapolskiy]

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 at 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 at 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 at mx31lite:~# dmesg | grep rtc0
[    2.682459] mc13xxx-rtc mc13783-rtc: rtc core: registered mc13783-rtc as rtc0

root at mx31lite:~# cat /proc/interrupts | grep mc13xxx-rtc
176:          0  spi32766.0  31 Edge      mc13xxx-rtc
177:          0  spi32766.0  25 Edge      mc13xxx-rtc

root at mx31lite:~# echo +5 > /sys/class/rtc/rtc0/wakealarm ; sleep 5

root at 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.

>> 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 :-)
> 

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.

--
With best wishes,
Vladimir

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Magnus Lilja]

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 at 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 at 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 at mx31lite:~# dmesg | grep rtc0
> [    2.682459] mc13xxx-rtc mc13783-rtc: rtc core: registered mc13783-rtc as rtc0
>
> root at mx31lite:~# cat /proc/interrupts | grep mc13xxx-rtc
> 176:          0  spi32766.0  31 Edge      mc13xxx-rtc
> 177:          0  spi32766.0  25 Edge      mc13xxx-rtc
>
> root at mx31lite:~# echo +5 > /sys/class/rtc/rtc0/wakealarm ; sleep 5
>
> root at 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

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Fabio Estevam]

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

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Fabio Estevam]

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

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Fabio Estevam]

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.

[PATCH 2/2] mfd: mc13xxx: Pass the IRQF_TRIGGER_HIGH flag.

[Magnus Lilja]

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