[PATCH 1/3] pinctrl: Intel: add RX invertion config

[PATCH 1/3] pinctrl: Intel: add RX invertion config

[Qipeng Zha]

Some module need to enable RX invertion config on BXT platform.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 45 +++++++++++++++++++++++++++++++++++
 drivers/pinctrl/intel/pinctrl-intel.h |  8 +++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 85536b4..ded5378 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -472,6 +472,14 @@ static int intel_config_get(struct pinctrl_dev *pctldev, unsigned pin,
 
 		break;
 
+	case PIN_CONF_INTEL_INV_RX:
+		value = intel_get_padcfg(pctrl, pin, PADCFG0);
+		if (value & PADCFG0_RXINV)
+			arg = 1;
+		else
+			arg = 0;
+		break;
+
 	default:
 		return -ENOTSUPP;
 	}
@@ -549,6 +557,38 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned pin,
 	return ret;
 }
 
+
+static int intel_config_set_misc(struct intel_pinctrl *pctrl, unsigned pin,
+				 unsigned long config)
+{
+	unsigned param = pinconf_to_config_param(config);
+	unsigned arg = pinconf_to_config_argument(config);
+	void __iomem *padcfg0;
+	unsigned long flags;
+	int ret = 0;
+	u32 value;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+
+	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
+	value = readl(padcfg0);
+
+	if (param == PIN_CONF_INTEL_INV_RX) {
+		if (arg)
+			value |= PADCFG0_RXINV;
+		else
+			value &= ~PADCFG0_RXINV;
+	} else
+		ret = -EINVAL;
+
+	if (!ret)
+		writel(value, padcfg0);
+
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return ret;
+}
+
 static int intel_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 			  unsigned long *configs, unsigned nconfigs)
 {
@@ -567,6 +607,11 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 			if (ret)
 				return ret;
 			break;
+		case PIN_CONF_INTEL_INV_RX:
+			ret = intel_config_set_misc(pctrl, pin, configs[i]);
+			if (ret)
+				return ret;
+			break;
 
 		default:
 			return -ENOTSUPP;
diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index b602157..51f9076 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -18,6 +18,14 @@ struct platform_device;
 struct device;
 
 /**
+* customized pinconf configurations
+* @PIN_CONF_INTEL_INV_RX: this will invert the RX pad state before it is
+*	sent to the GPIO-to-IOxAPIC, GPE/SCI, SMI, NMI logic or GPI_IS[n]
+*	that is using it.
+*/
+#define	PIN_CONF_INTEL_INV_RX		(PIN_CONFIG_END + 1)
+
+/**
  * struct intel_pingroup - Description about group of pins
  * @name: Name of the groups
  * @pins: All pins in this group
-- 
1.8.3.2

Re: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Mika Westerberg]

On Sat, Mar 12, 2016 at 01:06:00AM +0800, Qipeng Zha wrote:
> Some module need to enable RX invertion config on BXT platform.
> 
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>

Can I ask what module might need this? I'm not so certain we want to add
anything like this to the driver to be honest.

RE: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Zheng, Qi]

On Sat, Mar 12, 2016 at 01:06:00AM +0800, Qipeng Zha wrote:
> Some module need to enable RX invertion config on BXT platform.
> 
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>

> Can I ask what module might need this? I'm not so certain we want to add anything like this to the driver to be honest.

The "pi330532" device on Broxton requires this function to manually trigger an GPIO input interrupt.
That is to say, actually no signal change on the pin but toggling this RX inversion bit to make the GPIO controller believes there is voltage change on the pin thus triggering the interrupt.

Re: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Westerberg, Mika]

On Mon, Mar 14, 2016 at 03:10:07AM +0200, Zheng, Qi wrote:
> On Sat, Mar 12, 2016 at 01:06:00AM +0800, Qipeng Zha wrote:
> > Some module need to enable RX invertion config on BXT platform.
> > 
> > Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> 
> > Can I ask what module might need this? I'm not so certain we want to add anything like this to the driver to be honest.
> 
> The "pi330532" device on Broxton requires this function to manually trigger an GPIO input interrupt.
>
> That is to say, actually no signal change on the pin but toggling this
> RX inversion bit to make the GPIO controller believes there is voltage
> change on the pin thus triggering the interrupt.

That sounds rather hackish thing and definitely should not be part of
the driver IMHO.

I'm pretty sure there are better ways to do what you are doing.

RE: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Zheng, Qi]

BRs


On Mon, Mar 14, 2016 at 03:10:07AM +0200, Zheng, Qi wrote:
> On Sat, Mar 12, 2016 at 01:06:00AM +0800, Qipeng Zha wrote:
> > Some module need to enable RX invertion config on BXT platform.
> > 
> > Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> 
> > Can I ask what module might need this? I'm not so certain we want to add anything like this to the driver to be honest.
> 
> The "pi330532" device on Broxton requires this function to manually trigger an GPIO input interrupt.
>
> That is to say, actually no signal change on the pin but toggling this 
> RX inversion bit to make the GPIO controller believes there is voltage 
> change on the pin thus triggering the interrupt.

> That sounds rather hackish thing and definitely should not be part of the driver IMHO.

> I'm pretty sure there are better ways to do what you are doing.
We have gone through this requirement from CHT to BXT, there was no other better way to simulate the GPIO interrupt for the use of those devices.
If you insist it is not OK,  I am fine to keep it on Intel local branch.
Thanks.

Re: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Linus Walleij]

On Mon, Mar 14, 2016 at 9:56 AM, Zheng, Qi <qi.zheng@intel.com> wrote:

> The "pi330532" device on Broxton requires this function to manually
> trigger an GPIO input interrupt.
(...)
> We have gone through this requirement from CHT to BXT, there was no
> other better way to simulate the GPIO interrupt for the use of those devices.

I what you want is to trigger IRQs on GPIO lines using software
we need to add that to the GPIOlib subsystem, so this register
gets accessed from the GPIO side of things, not through pin control
I think?

We have so many diverse function pointers in the gpiochip, so
ability to trigger/test IRQs from software is certainly not a burden.

I don't understand the real-world usecase though, please explain
what kind of problem this is trying to overcome? Why does this
pi330532 driver need to do that, why can it not just inform the
driver that needs this interrupt that it should wake up, e.g by
using a notification or just an open-coded function call or whatever?

Yours,
Linus Walleij

RE: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Zheng, Qi]

> On Mon, Mar 14, 2016 at 9:56 AM, Zheng, Qi <qi.zheng@intel.com> wrote:

> The "pi330532" device on Broxton requires this function to manually 
> trigger an GPIO input interrupt.
(...)
> We have gone through this requirement from CHT to BXT, there was no 
> other better way to simulate the GPIO interrupt for the use of those devices.

> I what you want is to trigger IRQs on GPIO lines using software we need to add that to the GPIOlib subsystem, so this register gets accessed from the GPIO side of things, not through pin control I think?

We have pinctrl control map locally.
The RX inversion is implemented by pinctrl control calls, pinctrl_pm_select_default_state and pinctrl_pm_select_sleep_state. 

> We have so many diverse function pointers in the gpiochip, so ability to trigger/test IRQs from software is certainly not a burden.

> I don't understand the real-world usecase though, please explain what kind of problem this is trying to overcome? Why does this
> pi330532 driver need to do that, why can it not just inform the driver that needs this interrupt that it should wake up, e.g by using a notification or just an open-coded function call or whatever?

According to the pi330532 driver owner, 
"
we needed this support to simulate the HPD interrupt behavior as we don’t have dedicated interrupt line for Type-C DP HPD.

We don’t have any notifications mechasism b/w USB and display/Gfx stack and also not the ideal way to handle.  HPD toggling is the preferred approach suggested by VPG and HW teams to meet timing requirements also.

static void hpd_trigger(struct pi3usb30532_mux *chip, int state)
{
        dev_info(&chip->client->dev, "[HPD trigger] state : %d\n", state);

        if (state)
                pinctrl_pm_select_default_state(chip->dev);
        else
                pinctrl_pm_select_sleep_state(chip->dev);
}

"

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Linus Walleij]

On Tue, Mar 15, 2016 at 3:17 AM, Zheng, Qi <qi.zheng@intel.com> wrote:
>> On Mon, Mar 14, 2016 at 9:56 AM, Zheng, Qi <qi.zheng@intel.com> wrote:
>
>> The "pi330532" device on Broxton requires this function to manually
>> trigger an GPIO input interrupt.
> (...)
>> We have gone through this requirement from CHT to BXT, there was no
>> other better way to simulate the GPIO interrupt for the use of those devices.
>
>> I what you want is to trigger IRQs on GPIO lines using software we
>> need to add that to the GPIOlib subsystem, so this register gets accessed
>> from the GPIO side of things, not through pin control I think?
>
> We have pinctrl control map locally.
> The RX inversion is implemented by pinctrl control calls, pinctrl_pm_select_default_state
> and pinctrl_pm_select_sleep_state.
>
>> We have so many diverse function pointers in the gpiochip, so ability to
>> trigger/test IRQs from software is certainly not a burden.
>
>> I don't understand the real-world usecase though, please explain what kind of problem
>> this is trying to overcome? Why does this pi330532 driver need to do that, why can
>> it not just inform the driver that needs this interrupt that it should wake up, e.g by
>> using a notification or just an open-coded function call or whatever?
>
> According to the pi330532 driver owner,
> "
> we needed this support to simulate the HPD interrupt behavior as we don’t have
> dedicated interrupt line for Type-C DP HPD.

- What is a HPD interrupt?
- What is a Type-C DP HPD?
- Again why can't you just use a notifier or function call?

> We don’t have any notifications mechasism b/w USB and display/Gfx stack and
> also not the ideal way to handle.  HPD toggling is the preferred approach suggested
> by VPG and HW teams to meet timing requirements also.

What is VPG? Now it seems Intel's internal organization is being used as
part of the argument to get this change in and that makes me a bit
annoyed.

If there is no good notification mechanism then implement one instead
of starting to software-generate hardware interrupts.

I also start to get the feeling that these USB and display stacks
you are referring to are not the upstream versions.

> static void hpd_trigger(struct pi3usb30532_mux *chip, int state)
> {
>         dev_info(&chip->client->dev, "[HPD trigger] state : %d\n", state);
>
>         if (state)
>                 pinctrl_pm_select_default_state(chip->dev);
>         else
>                 pinctrl_pm_select_sleep_state(chip->dev);
> }

Can we get the *TECHNICAL* explanation of why this thing needs
to be done instead of using a notifier or function call?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Daniel Vetter]

On Wed, Mar 16, 2016 at 01:27:49PM +0100, Linus Walleij wrote:
> 
> On Tue, Mar 15, 2016 at 3:17 AM, Zheng, Qi <qi.zheng@intel.com> wrote:
> >> On Mon, Mar 14, 2016 at 9:56 AM, Zheng, Qi <qi.zheng@intel.com> wrote:
> >
> >> The "pi330532" device on Broxton requires this function to manually
> >> trigger an GPIO input interrupt.
> > (...)
> >> We have gone through this requirement from CHT to BXT, there was no
> >> other better way to simulate the GPIO interrupt for the use of those devices.
> >
> >> I what you want is to trigger IRQs on GPIO lines using software we
> >> need to add that to the GPIOlib subsystem, so this register gets accessed
> >> from the GPIO side of things, not through pin control I think?
> >
> > We have pinctrl control map locally.
> > The RX inversion is implemented by pinctrl control calls, pinctrl_pm_select_default_state
> > and pinctrl_pm_select_sleep_state.
> >
> >> We have so many diverse function pointers in the gpiochip, so ability to
> >> trigger/test IRQs from software is certainly not a burden.
> >
> >> I don't understand the real-world usecase though, please explain what kind of problem
> >> this is trying to overcome? Why does this pi330532 driver need to do that, why can
> >> it not just inform the driver that needs this interrupt that it should wake up, e.g by
> >> using a notification or just an open-coded function call or whatever?
> >
> > According to the pi330532 driver owner,
> > "
> > we needed this support to simulate the HPD interrupt behavior as we don’t have
> > dedicated interrupt line for Type-C DP HPD.
> 
> - What is a HPD interrupt?

hotplug interrupt, fires when you plug in a cable.

> - What is a Type-C DP HPD?

usb type C connector can multiplex both DisplayPort and USB, you need to
renegotiate the lane splitting every time the sink changes, i.e. on each
hotplug.

> - Again why can't you just use a notifier or function call?

Because windows sucks, hence all that coordination happens through hw
forwarded interrupts and magic registers. Same horror story on the sound
side, where the sound driver needs to know what kind of PCM stream the
monitor can take. It's hilarious. Except when they screw up the design and
then need to fake parts of it in software.

In sound we've switched over to a proper sw interface, and we tie the
different parts (drm graphics driver and alsa snd driver) using the
component.c framework.

> > We don’t have any notifications mechasism b/w USB and display/Gfx stack and
> > also not the ideal way to handle.  HPD toggling is the preferred approach suggested
> > by VPG and HW teams to meet timing requirements also.
> 
> What is VPG? Now it seems Intel's internal organization is being used as
> part of the argument to get this change in and that makes me a bit
> annoyed.
> 
> If there is no good notification mechanism then implement one instead
> of starting to software-generate hardware interrupts.
> 
> I also start to get the feeling that these USB and display stacks
> you are referring to are not the upstream versions.

There was chat of usb type C support for forever, but I was always
promised that we don't need any interactions on the sw side and it's all
magic in hw. There hasn't been any real design discussions in the open
source group. VPG is the hw/windows group and generally comes up with
"interesting" designs not suitable for upstream.

Feel free to just nack this stuff, and please cc intel-gfx/dri-devel in
the future (since I tend to ignore everything that's not cc'ed to mailing
lists I don't care about, even when I'm on cc personally). I've added them
all to cc.

Cheers, Daniel


> > static void hpd_trigger(struct pi3usb30532_mux *chip, int state)
> > {
> >         dev_info(&chip->client->dev, "[HPD trigger] state : %d\n", state);
> >
> >         if (state)
> >                 pinctrl_pm_select_default_state(chip->dev);
> >         else
> >                 pinctrl_pm_select_sleep_state(chip->dev);
> > }
> 
> Can we get the *TECHNICAL* explanation of why this thing needs
> to be done instead of using a notifier or function call?
> 
> Yours,
> Linus Walleij

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Linus Walleij]

On Wed, Mar 16, 2016 at 2:34 PM, Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org> wrote:
> On Wed, Mar 16, 2016 at 01:27:49PM +0100, Linus Walleij wrote:

>> - What is a HPD interrupt?
>
> hotplug interrupt, fires when you plug in a cable.
>
>> - What is a Type-C DP HPD?
>
> usb type C connector can multiplex both DisplayPort and USB, you need to
> renegotiate the lane splitting every time the sink changes, i.e. on each
> hotplug.

OK I understand, thanks a lot!

>> - Again why can't you just use a notifier or function call?
>
> Because windows sucks, hence all that coordination happens through hw
> forwarded interrupts and magic registers. Same horror story on the sound
> side, where the sound driver needs to know what kind of PCM stream the
> monitor can take. It's hilarious. Except when they screw up the design and
> then need to fake parts of it in software.

So the story is something like that these IRQs have been put into
hardware in order to compensate for flaws in Windows device driver
model, I see.

If there are such special registers in some hardware I guess I'm all for
implementing some generic quirk in gpiolib for people who need to
software-trigger GPIO IRQs. Could be good for testing too, as there
are such registers in ARMs PL061 GPIO controller for test, just so as
to simulate a GPIO IRQ.

gpiod_trig_irq() would work with me, I'm happy to support whatever
the GPIO hardware can do usually.

> In sound we've switched over to a proper sw interface, and we tie the
> different parts (drm graphics driver and alsa snd driver) using the
> component.c framework.

Hm is that solution or something similar proper for USB connector
as well I wonder... I was thinking about just adding $random_notifier
but maybe that is a bit ugly :/

>> What is VPG? Now it seems Intel's internal organization is being used as
>> part of the argument to get this change in and that makes me a bit
>> annoyed.
(...)
> There was chat of usb type C support for forever, but I was always
> promised that we don't need any interactions on the sw side and it's all
> magic in hw. There hasn't been any real design discussions in the open
> source group. VPG is the hw/windows group and generally comes up with
> "interesting" designs not suitable for upstream.
>
> Feel free to just nack this stuff, and please cc intel-gfx/dri-devel in
> the future (since I tend to ignore everything that's not cc'ed to mailing
> lists I don't care about, even when I'm on cc personally). I've added them
> all to cc.

Thanks a lot Daniel, I understand better now. I'm not really against
adding this "interesting" workaround if that is how Windows works,
we usually have to go by their standards. From the GPIO point
of view it is OK, just something the GPIO can do. I would be more
worried about what the USB PHY maintainer (Felipe) is going to say
about this.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Intel-gfx] [PATCH 1/3] pinctrl: Intel: add RX invertion config

[Jani Nikula]

On Thu, 17 Mar 2016, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Mar 16, 2016 at 2:34 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Mar 16, 2016 at 01:27:49PM +0100, Linus Walleij wrote:
>
>>> - What is a HPD interrupt?
>>
>> hotplug interrupt, fires when you plug in a cable.
>>
>>> - What is a Type-C DP HPD?
>>
>> usb type C connector can multiplex both DisplayPort and USB, you need to
>> renegotiate the lane splitting every time the sink changes, i.e. on each
>> hotplug.
>
> OK I understand, thanks a lot!
>
>>> - Again why can't you just use a notifier or function call?
>>
>> Because windows sucks, hence all that coordination happens through hw
>> forwarded interrupts and magic registers. Same horror story on the sound
>> side, where the sound driver needs to know what kind of PCM stream the
>> monitor can take. It's hilarious. Except when they screw up the design and
>> then need to fake parts of it in software.
>
> So the story is something like that these IRQs have been put into
> hardware in order to compensate for flaws in Windows device driver
> model, I see.
>
> If there are such special registers in some hardware I guess I'm all for
> implementing some generic quirk in gpiolib for people who need to
> software-trigger GPIO IRQs. Could be good for testing too, as there
> are such registers in ARMs PL061 GPIO controller for test, just so as
> to simulate a GPIO IRQ.
>
> gpiod_trig_irq() would work with me, I'm happy to support whatever
> the GPIO hardware can do usually.
>
>> In sound we've switched over to a proper sw interface, and we tie the
>> different parts (drm graphics driver and alsa snd driver) using the
>> component.c framework.
>
> Hm is that solution or something similar proper for USB connector
> as well I wonder... I was thinking about just adding $random_notifier
> but maybe that is a bit ugly :/
>
>>> What is VPG? Now it seems Intel's internal organization is being used as
>>> part of the argument to get this change in and that makes me a bit
>>> annoyed.
> (...)
>> There was chat of usb type C support for forever, but I was always
>> promised that we don't need any interactions on the sw side and it's all
>> magic in hw. There hasn't been any real design discussions in the open
>> source group. VPG is the hw/windows group and generally comes up with
>> "interesting" designs not suitable for upstream.
>>
>> Feel free to just nack this stuff, and please cc intel-gfx/dri-devel in
>> the future (since I tend to ignore everything that's not cc'ed to mailing
>> lists I don't care about, even when I'm on cc personally). I've added them
>> all to cc.
>
> Thanks a lot Daniel, I understand better now. I'm not really against
> adding this "interesting" workaround if that is how Windows works,
> we usually have to go by their standards. From the GPIO point
> of view it is OK, just something the GPIO can do. I would be more
> worried about what the USB PHY maintainer (Felipe) is going to say
> about this.

Adding Felipe's current address. Considering the new domain part of the
address, I'm hopeful we can sort this out. ;)

BR,
Jani.

>
> Yours,
> Linus Walleij
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Qipeng Zha]

There is one unexpected GPIO interrupt coming in below scenario.
1. GPIO X is going to be used as falling edge interrupt.
2. Before request_irq call, this GPIO X interrupt was masked.
3. But the IRQ mode may be set for some mode in default (by BIOS).
4. Toggle GPIO X from high to low.
5. The GPIO X interrupt status will be set even if it was masked.
6. Register the interrupt for GPIO X, the interrupt will be unmasked.
7. Even if no change on GPIO X afterwards, but one GPIO X interrupt
will be triggered because the interrupt status was set.

To avoid above issue, the interrupt status need clear before request_irq.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index ded5378..d6fe659 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
 		return -EPERM;
 	}
 
+	intel_gpio_irq_ack(d);
+
 	spin_lock_irqsave(&pctrl->lock, flags);
 
 	value = readl(reg);
-- 
1.8.3.2

Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Mika Westerberg]

On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote:
> There is one unexpected GPIO interrupt coming in below scenario.
> 1. GPIO X is going to be used as falling edge interrupt.
> 2. Before request_irq call, this GPIO X interrupt was masked.
> 3. But the IRQ mode may be set for some mode in default (by BIOS).
> 4. Toggle GPIO X from high to low.
> 5. The GPIO X interrupt status will be set even if it was masked.
> 6. Register the interrupt for GPIO X, the interrupt will be unmasked.
> 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt
> will be triggered because the interrupt status was set.
> 
> To avoid above issue, the interrupt status need clear before request_irq.
> 
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index ded5378..d6fe659 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
>  		return -EPERM;
>  	}
>  
> +	intel_gpio_irq_ack(d);

If the pin toggles right here, we still have the same issue, no?

We should check in the interrupt handler whether the interrupt is
actually enabled which I think we do already. Maybe that code has bug
somewhere?

> +
>  	spin_lock_irqsave(&pctrl->lock, flags);
>  
>  	value = readl(reg);
> -- 
> 1.8.3.2

RE: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Zheng, Qi]

On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote:
> There is one unexpected GPIO interrupt coming in below scenario.
> 1. GPIO X is going to be used as falling edge interrupt.
> 2. Before request_irq call, this GPIO X interrupt was masked.
> 3. But the IRQ mode may be set for some mode in default (by BIOS).
> 4. Toggle GPIO X from high to low.
> 5. The GPIO X interrupt status will be set even if it was masked.
> 6. Register the interrupt for GPIO X, the interrupt will be unmasked.
> 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt 
> will be triggered because the interrupt status was set.
> 
> To avoid above issue, the interrupt status need clear before request_irq.
> 
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c 
> b/drivers/pinctrl/intel/pinctrl-intel.c
> index ded5378..d6fe659 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
>  		return -EPERM;
>  	}
>  
> +	intel_gpio_irq_ack(d);

> If the pin toggles right here, we still have the same issue, no?

Yes. But it is very short time from here to the place IRQ got enabled.
To me, it is the only available platform dependent interface to do the "ACK" in the flow of request_irq.
Maybe you have other better option here?

> We should check in the interrupt handler whether the interrupt is actually enabled which I think we do already. Maybe that code has bug somewhere?

The check in the interrupt handler can't distinguish if the interrupt status was set before or after the request_irq.
On the Broxton RVP, one unexpected GPIO interrupt was captured during our GPIO unit test program.
This issue can be fixed by this patch.

> +
>  	spin_lock_irqsave(&pctrl->lock, flags);
>  
>  	value = readl(reg);
> --
> 1.8.3.2

Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Westerberg, Mika]

On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote:
> 
> On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote:
> > There is one unexpected GPIO interrupt coming in below scenario.
> > 1. GPIO X is going to be used as falling edge interrupt.
> > 2. Before request_irq call, this GPIO X interrupt was masked.
> > 3. But the IRQ mode may be set for some mode in default (by BIOS).
> > 4. Toggle GPIO X from high to low.
> > 5. The GPIO X interrupt status will be set even if it was masked.
> > 6. Register the interrupt for GPIO X, the interrupt will be unmasked.
> > 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt 
> > will be triggered because the interrupt status was set.
> > 
> > To avoid above issue, the interrupt status need clear before request_irq.
> > 
> > Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > ---
> >  drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c 
> > b/drivers/pinctrl/intel/pinctrl-intel.c
> > index ded5378..d6fe659 100644
> > --- a/drivers/pinctrl/intel/pinctrl-intel.c
> > +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> > @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
> >  		return -EPERM;
> >  	}
> >  
> > +	intel_gpio_irq_ack(d);
> 
> > If the pin toggles right here, we still have the same issue, no?
> 
> Yes. But it is very short time from here to the place IRQ got enabled.

That is still a race.

> To me, it is the only available platform dependent interface to do the "ACK" in the flow of request_irq.
> Maybe you have other better option here?
> 
> > We should check in the interrupt handler whether the interrupt is actually enabled which I think we do already. Maybe that code has bug somewhere?
> 
> The check in the interrupt handler can't distinguish if the interrupt status was set before or after the request_irq.
> On the Broxton RVP, one unexpected GPIO interrupt was captured during our GPIO unit test program.
> This issue can be fixed by this patch.

Drivers need to deal with the fact that they might get spurious
interrupts from time to time. You need to check in the interrupt handler
if the interrupt was from the device you are driving or not.

request_irq() enables the interrupt line and if the pin is already in
a state that triggers an interrupt, the driver interrupt handler will
be called.

RE: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Zheng, Qi]

On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote:
> 
> On Sat, Mar 12, 2016 at 01:06:01AM +0800, Qipeng Zha wrote:
> > There is one unexpected GPIO interrupt coming in below scenario.
> > 1. GPIO X is going to be used as falling edge interrupt.
> > 2. Before request_irq call, this GPIO X interrupt was masked.
> > 3. But the IRQ mode may be set for some mode in default (by BIOS).
> > 4. Toggle GPIO X from high to low.
> > 5. The GPIO X interrupt status will be set even if it was masked.
> > 6. Register the interrupt for GPIO X, the interrupt will be unmasked.
> > 7. Even if no change on GPIO X afterwards, but one GPIO X interrupt 
> > will be triggered because the interrupt status was set.
> > 
> > To avoid above issue, the interrupt status need clear before request_irq.
> > 
> > Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > ---
> >  drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
> > b/drivers/pinctrl/intel/pinctrl-intel.c
> > index ded5378..d6fe659 100644
> > --- a/drivers/pinctrl/intel/pinctrl-intel.c
> > +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> > @@ -773,6 +773,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
> >  		return -EPERM;
> >  	}
> >  
> > +	intel_gpio_irq_ack(d);
> 
> > If the pin toggles right here, we still have the same issue, no?
> 
> Yes. But it is very short time from here to the place IRQ got enabled.

>That is still a race.

> To me, it is the only available platform dependent interface to do the "ACK" in the flow of request_irq.
> Maybe you have other better option here?
> 
> > We should check in the interrupt handler whether the interrupt is actually enabled which I think we do already. Maybe that code has bug somewhere?
> 
> The check in the interrupt handler can't distinguish if the interrupt status was set before or after the request_irq.
> On the Broxton RVP, one unexpected GPIO interrupt was captured during our GPIO unit test program.
> This issue can be fixed by this patch.

> Drivers need to deal with the fact that they might get spurious interrupts from time to time. You need to check in the interrupt handler if the interrupt was from the device you are driving or not.

> request_irq() enables the interrupt line and if the pin is already in a state that triggers an interrupt, the driver interrupt handler will be called.

I agree with you that the driver need deal with spurious interrupts, but it is expected that the GPIO driver eliminate this spurious interrupts as well which the patch can help.
And IMHO, adding ACK in the irq_set_type makes sense since a clean interrupt status is expected behavior before enabling the interrupt.

Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Westerberg, Mika]

On Mon, Mar 14, 2016 at 11:02:04AM +0200, Zheng, Qi wrote:
> I agree with you that the driver need deal with spurious interrupts,
> but it is expected that the GPIO driver eliminate this spurious
> interrupts as well which the patch can help.  And IMHO, adding ACK in
> the irq_set_type makes sense since a clean interrupt status is
> expected behavior before enabling the interrupt.

But it does not solve anything if the pin status changes right after you
call ack.

Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Linus Walleij]

On Mon, Mar 14, 2016 at 9:44 AM, Westerberg, Mika
<mika.westerberg@intel.com> wrote:
> On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote:

>> > +   intel_gpio_irq_ack(d);
>>
>> > If the pin toggles right here, we still have the same issue, no?
>>
>> Yes. But it is very short time from here to the place IRQ got enabled.
>
> That is still a race.
>
>> To me, it is the only available platform dependent interface to do
> the "ACK" in the flow of request_irq. Maybe you have other better
> option here?
(...)
> Drivers need to deal with the fact that they might get spurious
> interrupts from time to time. You need to check in the interrupt handler
> if the interrupt was from the device you are driving or not.
>
> request_irq() enables the interrupt line and if the pin is already in
> a state that triggers an interrupt, the driver interrupt handler will
> be called.

This looks like something is wrong in the irqchip.

Your set_type() is supporting edges but have all IRQs
handled by handle_simple_irq() rather than handle_edge_irq()
for the edges, which gives a more robust control flow
from IRQ to ACK to calling the handler.

Zheng/Mika: please look at how the level/edge
IRQs are handled in drivers/gpio/gpio-pl061.c
where I *tried* to do things right, switching handler
in .set_type() using irq_set_handler_locked(). I think
you may need to use handle_edge_irq() for the edge IRQs
and handle_level_irq() for the level IRQs just like I do
in the PL061 driver.

(I might be wrong.)

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Westerberg, Mika]

On Mon, Mar 14, 2016 at 01:40:39PM +0100, Linus Walleij wrote:
> On Mon, Mar 14, 2016 at 9:44 AM, Westerberg, Mika
> <mika.westerberg@intel.com> wrote:
> > On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote:
> 
> >> > +   intel_gpio_irq_ack(d);
> >>
> >> > If the pin toggles right here, we still have the same issue, no?
> >>
> >> Yes. But it is very short time from here to the place IRQ got enabled.
> >
> > That is still a race.
> >
> >> To me, it is the only available platform dependent interface to do
> > the "ACK" in the flow of request_irq. Maybe you have other better
> > option here?
> (...)
> > Drivers need to deal with the fact that they might get spurious
> > interrupts from time to time. You need to check in the interrupt handler
> > if the interrupt was from the device you are driving or not.
> >
> > request_irq() enables the interrupt line and if the pin is already in
> > a state that triggers an interrupt, the driver interrupt handler will
> > be called.
> 
> This looks like something is wrong in the irqchip.

If request_irq() is called so that the interrupt is level triggered,
let's say active low, and the pin is already in that state I would
expect interrupt to trigger immediately when enabled, no?

If I understand correctly this is precisely what Zheng is describing
they are trying to solve.

> Your set_type() is supporting edges but have all IRQs
> handled by handle_simple_irq() rather than handle_edge_irq()
> for the edges, which gives a more robust control flow
> from IRQ to ACK to calling the handler.
> 
> Zheng/Mika: please look at how the level/edge
> IRQs are handled in drivers/gpio/gpio-pl061.c
> where I *tried* to do things right, switching handler
> in .set_type() using irq_set_handler_locked(). I think
> you may need to use handle_edge_irq() for the edge IRQs
> and handle_level_irq() for the level IRQs just like I do
> in the PL061 driver.

The driver is already doing that as far as I can tell (see
intel_gpio_irq_type()).

Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Westerberg, Mika]

On Mon, Mar 14, 2016 at 02:54:36PM +0200, Westerberg, Mika wrote:
> On Mon, Mar 14, 2016 at 01:40:39PM +0100, Linus Walleij wrote:
> > On Mon, Mar 14, 2016 at 9:44 AM, Westerberg, Mika
> > <mika.westerberg@intel.com> wrote:
> > > On Mon, Mar 14, 2016 at 03:24:06AM +0200, Zheng, Qi wrote:
> > 
> > >> > +   intel_gpio_irq_ack(d);
> > >>
> > >> > If the pin toggles right here, we still have the same issue, no?
> > >>
> > >> Yes. But it is very short time from here to the place IRQ got enabled.
> > >
> > > That is still a race.
> > >
> > >> To me, it is the only available platform dependent interface to do
> > > the "ACK" in the flow of request_irq. Maybe you have other better
> > > option here?
> > (...)
> > > Drivers need to deal with the fact that they might get spurious
> > > interrupts from time to time. You need to check in the interrupt handler
> > > if the interrupt was from the device you are driving or not.
> > >
> > > request_irq() enables the interrupt line and if the pin is already in
> > > a state that triggers an interrupt, the driver interrupt handler will
> > > be called.
> > 
> > This looks like something is wrong in the irqchip.
> 
> If request_irq() is called so that the interrupt is level triggered,
> let's say active low, and the pin is already in that state I would
> expect interrupt to trigger immediately when enabled, no?
> 
> If I understand correctly this is precisely what Zheng is describing
> they are trying to solve.

Scratch that. I just re-read the patch changelog and they are
configuring the pin as edge triggered.

> > Your set_type() is supporting edges but have all IRQs
> > handled by handle_simple_irq() rather than handle_edge_irq()
> > for the edges, which gives a more robust control flow
> > from IRQ to ACK to calling the handler.
> > 
> > Zheng/Mika: please look at how the level/edge
> > IRQs are handled in drivers/gpio/gpio-pl061.c
> > where I *tried* to do things right, switching handler
> > in .set_type() using irq_set_handler_locked(). I think
> > you may need to use handle_edge_irq() for the edge IRQs
> > and handle_level_irq() for the level IRQs just like I do
> > in the PL061 driver.
> 
> The driver is already doing that as far as I can tell (see
> intel_gpio_irq_type()).

I will check again if we are still missing something there.

Re: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Westerberg, Mika]

On Mon, Mar 14, 2016 at 03:00:41PM +0200, Westerberg, Mika wrote:
> 
> > > Your set_type() is supporting edges but have all IRQs
> > > handled by handle_simple_irq() rather than handle_edge_irq()
> > > for the edges, which gives a more robust control flow
> > > from IRQ to ACK to calling the handler.
> > > 
> > > Zheng/Mika: please look at how the level/edge
> > > IRQs are handled in drivers/gpio/gpio-pl061.c
> > > where I *tried* to do things right, switching handler
> > > in .set_type() using irq_set_handler_locked(). I think
> > > you may need to use handle_edge_irq() for the edge IRQs
> > > and handle_level_irq() for the level IRQs just like I do
> > > in the PL061 driver.
> > 
> > The driver is already doing that as far as I can tell (see
> > intel_gpio_irq_type()).
> 
> I will check again if we are still missing something there.

Maybe we can implement ->enable() that clears the status right before
interrupt is unmasked? Something like below.

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index c0f5586218c4..b4873a4e25d5 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -648,6 +648,33 @@ static const struct gpio_chip intel_gpio_chip = {
 	.set = intel_gpio_set,
 };
 
+static void intel_gpio_irq_enable(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct intel_community *community;
+	unsigned pin = irqd_to_hwirq(d);
+	unsigned long flags;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+
+	community = intel_get_community(pctrl, pin);
+	if (community) {
+		unsigned padno = pin_to_padno(community, pin);
+		unsigned gpp_offset = padno % community->gpp_size;
+		unsigned gpp = padno / community->gpp_size;
+		unsigned value;
+
+		writel(BIT(gpp_offset), community->regs + GPI_IS + gpp * 4);
+
+		value = readl(community->regs + community->ie_offset + gpp * 4);
+		value |= BIT(gpp_offset);
+		writel(value, community->regs + community->ie_offset + gpp * 4);
+	}
+
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
 static void intel_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -856,6 +883,7 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
 
 static struct irq_chip intel_gpio_irqchip = {
 	.name = "intel-gpio",
+	.irq_enable = intel_gpio_irq_enable,
 	.irq_ack = intel_gpio_irq_ack,
 	.irq_mask = intel_gpio_irq_mask,
 	.irq_unmask = intel_gpio_irq_unmask,

RE: [PATCH 2/3] pinctrl:Intel: clear interrupt status for every IRQ setup

[Zheng, Qi]

> On Mon, Mar 14, 2016 at 03:00:41PM +0200, Westerberg, Mika wrote:
> 
> > > Your set_type() is supporting edges but have all IRQs handled by 
> > > handle_simple_irq() rather than handle_edge_irq() for the edges, 
> > > which gives a more robust control flow from IRQ to ACK to calling 
> > > the handler.
> > > 
> > > Zheng/Mika: please look at how the level/edge IRQs are handled in 
> > > drivers/gpio/gpio-pl061.c where I *tried* to do things right, 
> > > switching handler in .set_type() using irq_set_handler_locked(). I 
> > > think you may need to use handle_edge_irq() for the edge IRQs and 
> > > handle_level_irq() for the level IRQs just like I do in the PL061 
> > > driver.
> > 
> > The driver is already doing that as far as I can tell (see 
> > intel_gpio_irq_type()).
> 
> I will check again if we are still missing something there.

> Maybe we can implement ->enable() that clears the status right before interrupt is unmasked? Something like below.
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index c0f5586218c4..b4873a4e25d5 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -648,6 +648,33 @@ static const struct gpio_chip intel_gpio_chip = {
 > 	.set = intel_gpio_set,
 > };
 >

The gpio_irq_enable callback way looks better.
I will do the test with this solution and submit it if the unit test passed (no unexpected gpio interrupt).
Thanks.

[PATCH 3/3] pinctrl:Intel: make the high level interrupt working

[Qipeng Zha]

High level trigger mode of GPIO interrupt is not set correctly
in intel_gpio_irq_type(), and will make this kind of interrupt
not respond.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index d6fe659..706a21f 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -790,6 +790,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
 		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
 	} else if (type & IRQ_TYPE_LEVEL_LOW) {
 		value |= PADCFG0_RXINV;
+	} else if (type & IRQ_TYPE_LEVEL_HIGH) {
+		;
 	} else {
 		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
 	}
-- 
1.8.3.2

Re: [PATCH 3/3] pinctrl:Intel: make the high level interrupt working

[Mika Westerberg]

On Sat, Mar 12, 2016 at 01:06:02AM +0800, Qipeng Zha wrote:
> High level trigger mode of GPIO interrupt is not set correctly
> in intel_gpio_irq_type(), and will make this kind of interrupt
> not respond.

Good finding.

> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index d6fe659..706a21f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -790,6 +790,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
>  		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
>  	} else if (type & IRQ_TYPE_LEVEL_LOW) {
>  		value |= PADCFG0_RXINV;
> +	} else if (type & IRQ_TYPE_LEVEL_HIGH) {
> +		;

What about following instead?

	} else if (type & IRQ_TYPE_LEVEL_MASK) {
		if (type & IRQ_TYPE_LEVEL_LOW)
			value |= PADCFG0_RXINV;
	}


>  	} else {
>  		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
>  	}
> -- 
> 1.8.3.2

RE: [PATCH 3/3] pinctrl:Intel: make the high level interrupt working

[Zheng, Qi]

On Sat, Mar 12, 2016 at 01:06:02AM +0800, Qipeng Zha wrote:
> High level trigger mode of GPIO interrupt is not set correctly in 
> intel_gpio_irq_type(), and will make this kind of interrupt not 
> respond.

> Good finding.

> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c 
> b/drivers/pinctrl/intel/pinctrl-intel.c
> index d6fe659..706a21f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -790,6 +790,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
>  		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
>  	} else if (type & IRQ_TYPE_LEVEL_LOW) {
>  		value |= PADCFG0_RXINV;
> +	} else if (type & IRQ_TYPE_LEVEL_HIGH) {
> +		;

> What about following instead?

Nothing need to do.
Because the default setting below  in the intel_gpio_irq_type is for high level mode interrupt. 
	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);

>
>	} else if (type & IRQ_TYPE_LEVEL_MASK) {
>		if (type & IRQ_TYPE_LEVEL_LOW)
>			value |= PADCFG0_RXINV;
>	}



>  	} else {
>  		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
>  	}
> --
> 1.8.3.2

RE: [PATCH 3/3] pinctrl:Intel: make the high level interrupt working

[Zheng, Qi]

On Sat, Mar 12, 2016 at 01:06:02AM +0800, Qipeng Zha wrote:
> High level trigger mode of GPIO interrupt is not set correctly in 
> intel_gpio_irq_type(), and will make this kind of interrupt not 
> respond.

> Good finding.

> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
> b/drivers/pinctrl/intel/pinctrl-intel.c
> index d6fe659..706a21f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -790,6 +790,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
>  		value |= PADCFG0_RXEVCFG_EDGE << PADCFG0_RXEVCFG_SHIFT;
>  	} else if (type & IRQ_TYPE_LEVEL_LOW) {
>  		value |= PADCFG0_RXINV;
> +	} else if (type & IRQ_TYPE_LEVEL_HIGH) {
> +		;

> What about following instead?

> Nothing need to do.
> Because the default setting below  in the intel_gpio_irq_type is for high level mode interrupt. 
>	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);

Sorry, misunderstood your points.
The following looks fine to me.
Thanks.

>
>	} else if (type & IRQ_TYPE_LEVEL_MASK) {
>		if (type & IRQ_TYPE_LEVEL_LOW)
>			value |= PADCFG0_RXINV;
>	}



>  	} else {
>  		value |= PADCFG0_RXEVCFG_DISABLED << PADCFG0_RXEVCFG_SHIFT;
>  	}
> --
> 1.8.3.2