Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] pinctrl: qcom: Add irq_enable callback for msm gpio
@ 2019-06-17  9:34 Neeraj Upadhyay
  2019-06-25  8:58 ` Linus Walleij
  0 siblings, 1 reply; 4+ messages in thread
From: Neeraj Upadhyay @ 2019-06-17  9:34 UTC (permalink / raw)
  To: bjorn.andersson, sboyd, linus.walleij, timur
  Cc: linux-arm-msm, linux-gpio, linux-kernel, Srinivas Ramana,
	Neeraj Upadhyay

From: Srinivas Ramana <sramana@codeaurora.org>

Introduce the irq_enable callback which will be same as irq_unmask
except that it will also clear the status bit before unmask.

This will help in clearing any erroneous interrupts that would
have got latched when the interrupt is not in use.

There may be devices like UART which can use the same gpio line
for data rx as well as a wakeup gpio when in suspend. The data that
was flowing on the line may latch the interrupt and when we enable
the interrupt before going to suspend, this would trigger the
unexpected interrupt. This change helps clearing the interrupt
so that these unexpected interrupts gets cleared.

Signed-off-by: Srinivas Ramana <sramana@codeaurora.org>
Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
---

Changes since v1:
- Extracted common code into __msm_gpio_irq_unmask().

 drivers/pinctrl/qcom/pinctrl-msm.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 6e319bc..2a127f0 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -729,7 +729,7 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }
 
-static void msm_gpio_irq_unmask(struct irq_data *d)
+static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
@@ -741,6 +741,17 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (status_clear) {
+		/*
+		 * clear the interrupt status bit before unmask to avoid
+		 * any erroneous interrupts that would have got latched
+		 * when the interrupt is not in use.
+		 */
+		val = msm_readl_intr_status(pctrl, g);
+		val &= ~BIT(g->intr_status_bit);
+		msm_writel_intr_status(val, pctrl, g);
+	}
+
 	val = msm_readl_intr_cfg(pctrl, g);
 	val |= BIT(g->intr_raw_status_bit);
 	val |= BIT(g->intr_enable_bit);
@@ -751,6 +762,17 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }
 
+static void msm_gpio_irq_enable(struct irq_data *d)
+{
+
+	__msm_gpio_irq_unmask(d, true);
+}
+
+static void msm_gpio_irq_unmask(struct irq_data *d)
+{
+	__msm_gpio_irq_unmask(d, false);
+}
+
 static void msm_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -978,6 +1000,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl);
 
 	pctrl->irq_chip.name = "msmgpio";
+	pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] pinctrl: qcom: Add irq_enable callback for msm gpio
  2019-06-17  9:34 [PATCH v2] pinctrl: qcom: Add irq_enable callback for msm gpio Neeraj Upadhyay
@ 2019-06-25  8:58 ` Linus Walleij
  2019-06-25 10:29   ` Neeraj Upadhyay
  0 siblings, 1 reply; 4+ messages in thread
From: Linus Walleij @ 2019-06-25  8:58 UTC (permalink / raw)
  To: Neeraj Upadhyay
  Cc: Bjorn Andersson, Stephen Boyd, Timur Tabi, MSM,
	open list:GPIO SUBSYSTEM, linux-kernel, Srinivas Ramana

On Mon, Jun 17, 2019 at 11:35 AM Neeraj Upadhyay <neeraju@codeaurora.org> wrote:

> From: Srinivas Ramana <sramana@codeaurora.org>
>
> Introduce the irq_enable callback which will be same as irq_unmask
> except that it will also clear the status bit before unmask.
>
> This will help in clearing any erroneous interrupts that would
> have got latched when the interrupt is not in use.
>
> There may be devices like UART which can use the same gpio line
> for data rx as well as a wakeup gpio when in suspend. The data that
> was flowing on the line may latch the interrupt and when we enable
> the interrupt before going to suspend, this would trigger the
> unexpected interrupt. This change helps clearing the interrupt
> so that these unexpected interrupts gets cleared.
>
> Signed-off-by: Srinivas Ramana <sramana@codeaurora.org>
> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>

Overall this looks good to me, waiting for Bjorn's review.

> Changes since v1:
> - Extracted common code into __msm_gpio_irq_unmask().

Please don't name functions __like __that.

> -static void msm_gpio_irq_unmask(struct irq_data *d)
> +static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear)

Instead of __unclear __underscore __semantic use something
really descriptive like

static void msm_gpio_irq_clear_irq()

That is what it does, right?

Other than that it looks fine.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] pinctrl: qcom: Add irq_enable callback for msm gpio
  2019-06-25  8:58 ` Linus Walleij
@ 2019-06-25 10:29   ` Neeraj Upadhyay
  2019-06-25 12:18     ` Linus Walleij
  0 siblings, 1 reply; 4+ messages in thread
From: Neeraj Upadhyay @ 2019-06-25 10:29 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Bjorn Andersson, Stephen Boyd, Timur Tabi, MSM,
	open list:GPIO SUBSYSTEM, linux-kernel, Srinivas Ramana


On 6/25/19 2:28 PM, Linus Walleij wrote:
> On Mon, Jun 17, 2019 at 11:35 AM Neeraj Upadhyay <neeraju@codeaurora.org> wrote:
>
>> From: Srinivas Ramana <sramana@codeaurora.org>
>>
>> Introduce the irq_enable callback which will be same as irq_unmask
>> except that it will also clear the status bit before unmask.
>>
>> This will help in clearing any erroneous interrupts that would
>> have got latched when the interrupt is not in use.
>>
>> There may be devices like UART which can use the same gpio line
>> for data rx as well as a wakeup gpio when in suspend. The data that
>> was flowing on the line may latch the interrupt and when we enable
>> the interrupt before going to suspend, this would trigger the
>> unexpected interrupt. This change helps clearing the interrupt
>> so that these unexpected interrupts gets cleared.
>>
>> Signed-off-by: Srinivas Ramana <sramana@codeaurora.org>
>> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> Overall this looks good to me, waiting for Bjorn's review.
>
>> Changes since v1:
>> - Extracted common code into __msm_gpio_irq_unmask().
> Please don't name functions __like __that.
>
>> -static void msm_gpio_irq_unmask(struct irq_data *d)
>> +static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear)
> Instead of __unclear __underscore __semantic use something
> really descriptive like
>
> static void msm_gpio_irq_clear_irq()
>
> That is what it does, right?

Is below ok? as it clears (if status_clear set) and then unmasks irq

static void msm_gpio_irq_clear_unmask()

>
> Other than that it looks fine.
>
> Yours,
> Linus Walleij

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] pinctrl: qcom: Add irq_enable callback for msm gpio
  2019-06-25 10:29   ` Neeraj Upadhyay
@ 2019-06-25 12:18     ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2019-06-25 12:18 UTC (permalink / raw)
  To: Neeraj Upadhyay
  Cc: Bjorn Andersson, Stephen Boyd, Timur Tabi, MSM,
	open list:GPIO SUBSYSTEM, linux-kernel, Srinivas Ramana

On Tue, Jun 25, 2019 at 12:29 PM Neeraj Upadhyay <neeraju@codeaurora.org> wrote:
> On 6/25/19 2:28 PM, Linus Walleij wrote:

> > Please don't name functions __like __that.
> >
> >> -static void msm_gpio_irq_unmask(struct irq_data *d)
> >> +static void __msm_gpio_irq_unmask(struct irq_data *d, bool status_clear)
> > Instead of __unclear __underscore __semantic use something
> > really descriptive like
> >
> > static void msm_gpio_irq_clear_irq()
> >
> > That is what it does, right?
>
> Is below ok? as it clears (if status_clear set) and then unmasks irq
>
> static void msm_gpio_irq_clear_unmask()

Sure thing! Thanks.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17  9:34 [PATCH v2] pinctrl: qcom: Add irq_enable callback for msm gpio Neeraj Upadhyay
2019-06-25  8:58 ` Linus Walleij
2019-06-25 10:29   ` Neeraj Upadhyay
2019-06-25 12:18     ` Linus Walleij

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox