In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
msm gpio") we tried to Ack interrupts during unmask. However, that
patch forgot to check "intr_ack_high" so, presumably, it only worked
for a certain subset of SoCs.
Let's add a small accessor so we don't need to open-code the logic in
both places.
This was found by code inspection. I don't have any access to the
hardware in question nor software that needs the Ack during unmask.
Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
Signed-off-by: Douglas Anderson <[email protected]>
---
It should be noted that this code will be moved in the next patch. In
theory this could be squashed into the next patch but it seems more
documenting to have this as a separate patch.
Changes in v5:
- ("pinctrl: qcom: Properly clear "intr_ack_high" interrupts...") new for v5.
drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 1787ada6bfab..a6b0c17e2f78 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
MSM_ACCESSOR(intr_status)
MSM_ACCESSOR(intr_target)
+static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
+ const struct msm_pingroup *g)
+{
+ u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
+
+ msm_writel_intr_status(val, pctrl, g);
+}
+
static int msm_get_groups_count(struct pinctrl_dev *pctldev)
{
struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -798,7 +806,7 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
* when the interrupt is not in use.
*/
if (status_clear)
- msm_writel_intr_status(0, pctrl, g);
+ msm_ack_intr_status(pctrl, g);
val = msm_readl_intr_cfg(pctrl, g);
val |= BIT(g->intr_raw_status_bit);
@@ -891,7 +899,6 @@ static void msm_gpio_irq_ack(struct irq_data *d)
struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
const struct msm_pingroup *g;
unsigned long flags;
- u32 val;
if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
@@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
raw_spin_lock_irqsave(&pctrl->lock, flags);
- val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
- msm_writel_intr_status(val, pctrl, g);
+ msm_ack_intr_status(pctrl, g);
if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
msm_gpio_update_dual_edge_pos(pctrl, g, d);
--
2.29.2.729.g45daf8777d-goog
Hi Doug,
Reviewed-by: Maulik Shah <[email protected]>
Tested-by: Maulik Shah <[email protected]>
Thanks,
Maulik
On 1/8/2021 11:05 PM, Douglas Anderson wrote:
> In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
> msm gpio") we tried to Ack interrupts during unmask. However, that
> patch forgot to check "intr_ack_high" so, presumably, it only worked
> for a certain subset of SoCs.
>
> Let's add a small accessor so we don't need to open-code the logic in
> both places.
>
> This was found by code inspection. I don't have any access to the
> hardware in question nor software that needs the Ack during unmask.
>
> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> It should be noted that this code will be moved in the next patch. In
> theory this could be squashed into the next patch but it seems more
> documenting to have this as a separate patch.
>
> Changes in v5:
> - ("pinctrl: qcom: Properly clear "intr_ack_high" interrupts...") new for v5.
>
> drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1787ada6bfab..a6b0c17e2f78 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
> MSM_ACCESSOR(intr_status)
> MSM_ACCESSOR(intr_target)
>
> +static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
> + const struct msm_pingroup *g)
> +{
> + u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> +
> + msm_writel_intr_status(val, pctrl, g);
> +}
> +
> static int msm_get_groups_count(struct pinctrl_dev *pctldev)
> {
> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -798,7 +806,7 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> * when the interrupt is not in use.
> */
> if (status_clear)
> - msm_writel_intr_status(0, pctrl, g);
> + msm_ack_intr_status(pctrl, g);
>
> val = msm_readl_intr_cfg(pctrl, g);
> val |= BIT(g->intr_raw_status_bit);
> @@ -891,7 +899,6 @@ static void msm_gpio_irq_ack(struct irq_data *d)
> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> const struct msm_pingroup *g;
> unsigned long flags;
> - u32 val;
>
> if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
> if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> @@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> - val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> - msm_writel_intr_status(val, pctrl, g);
> + msm_ack_intr_status(pctrl, g);
>
> if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> msm_gpio_update_dual_edge_pos(pctrl, g, d);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoting Douglas Anderson (2021-01-08 09:35:15)
> In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
> msm gpio") we tried to Ack interrupts during unmask. However, that
> patch forgot to check "intr_ack_high" so, presumably, it only worked
> for a certain subset of SoCs.
>
> Let's add a small accessor so we don't need to open-code the logic in
> both places.
>
> This was found by code inspection. I don't have any access to the
> hardware in question nor software that needs the Ack during unmask.
>
> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
One minor nit below.
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1787ada6bfab..a6b0c17e2f78 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
> MSM_ACCESSOR(intr_status)
> MSM_ACCESSOR(intr_target)
>
> +static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
> + const struct msm_pingroup *g)
> +{
> + u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
Would be nice to remove that extra parenthesis too.
> +
> + msm_writel_intr_status(val, pctrl, g);
> +}
> +
> static int msm_get_groups_count(struct pinctrl_dev *pctldev)
> {
> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> - val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
Even though it is here.
On Fri 08 Jan 11:35 CST 2021, Douglas Anderson wrote:
> In commit 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for
> msm gpio") we tried to Ack interrupts during unmask. However, that
> patch forgot to check "intr_ack_high" so, presumably, it only worked
> for a certain subset of SoCs.
>
> Let's add a small accessor so we don't need to open-code the logic in
> both places.
>
> This was found by code inspection. I don't have any access to the
> hardware in question nor software that needs the Ack during unmask.
>
> Fixes: 4b7618fdc7e6 ("pinctrl: qcom: Add irq_enable callback for msm gpio")
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
And I agree with Stephen's comment about the unnecessary parenthesis
around g->intr_ack_high.
Regards,
Bjorn
> ---
> It should be noted that this code will be moved in the next patch. In
> theory this could be squashed into the next patch but it seems more
> documenting to have this as a separate patch.
>
> Changes in v5:
> - ("pinctrl: qcom: Properly clear "intr_ack_high" interrupts...") new for v5.
>
> drivers/pinctrl/qcom/pinctrl-msm.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1787ada6bfab..a6b0c17e2f78 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -96,6 +96,14 @@ MSM_ACCESSOR(intr_cfg)
> MSM_ACCESSOR(intr_status)
> MSM_ACCESSOR(intr_target)
>
> +static void msm_ack_intr_status(struct msm_pinctrl *pctrl,
> + const struct msm_pingroup *g)
> +{
> + u32 val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> +
> + msm_writel_intr_status(val, pctrl, g);
> +}
> +
> static int msm_get_groups_count(struct pinctrl_dev *pctldev)
> {
> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -798,7 +806,7 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> * when the interrupt is not in use.
> */
> if (status_clear)
> - msm_writel_intr_status(0, pctrl, g);
> + msm_ack_intr_status(pctrl, g);
>
> val = msm_readl_intr_cfg(pctrl, g);
> val |= BIT(g->intr_raw_status_bit);
> @@ -891,7 +899,6 @@ static void msm_gpio_irq_ack(struct irq_data *d)
> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> const struct msm_pingroup *g;
> unsigned long flags;
> - u32 val;
>
> if (test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
> if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> @@ -903,8 +910,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> - val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> - msm_writel_intr_status(val, pctrl, g);
> + msm_ack_intr_status(pctrl, g);
>
> if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> msm_gpio_update_dual_edge_pos(pctrl, g, d);
> --
> 2.29.2.729.g45daf8777d-goog
>