On Fri 08 Jan 11:35 CST 2021, Douglas Anderson wrote:
> When the Qualcomm pinctrl driver wants to Ack an interrupt, it does a
> read-modify-write on the interrupt status register. On some SoCs it
> makes sure that the status bit is 1 to "Ack" and on others it makes
> sure that the bit is 0 to "Ack". Presumably the first type of
> interrupt controller is a "write 1 to clear" type register and the
> second just let you directly set the interrupt status register.
>
> As far as I can tell from scanning structure definitions, the
> interrupt status bit is always in a register by itself. Thus with
> both types of interrupt controllers it is safe to "Ack" interrupts
> without doing a read-modify-write. We can do a simple write.
>
> It should be noted that if the interrupt status bit _was_ ever in a
> register with other things (like maybe status bits for other GPIOs):
> a) For "write 1 clear" type controllers then read-modify-write would
> be totally wrong because we'd accidentally end up clearing
> interrupts we weren't looking at.
> b) For "direct set" type controllers then read-modify-write would also
> be wrong because someone setting one of the other bits in the
> register might accidentally clear (or set) our interrupt.
> I say this simply to show that the current read-modify-write doesn't
> provide any sort of "future proofing" of the code. In fact (for
> "write 1 clear" controllers) the new code is slightly more "future
> proof" since it would allow more than one interrupt status bits to
> share a register.
>
> NOTE: this code fixes no bugs--it simply avoids an extra register
> read.
>
> Signed-off-by: Douglas Anderson <diand...@chromium.org>
Reviewed-by: Bjorn Andersson <bjorn.anders...@linaro.org>
Regards,
Bjorn
> ---
>
> Changes in v5:
> - ("pinctrl: qcom: No need to read-modify-write the ...") new for v5.
>
> drivers/pinctrl/qcom/pinctrl-msm.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
> b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 1d2a78452c2d..1787ada6bfab 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -792,16 +792,13 @@ static void msm_gpio_irq_clear_unmask(struct irq_data
> *d, bool status_clear)
>
> 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);
> - }
> + /*
> + * clear the interrupt status bit before unmask to avoid
> + * any erroneous interrupts that would have got latched
> + * when the interrupt is not in use.
> + */
> + if (status_clear)
> + msm_writel_intr_status(0, pctrl, g);
>
> val = msm_readl_intr_cfg(pctrl, g);
> val |= BIT(g->intr_raw_status_bit);
> @@ -906,11 +903,7 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> - val = msm_readl_intr_status(pctrl, g);
> - if (g->intr_ack_high)
> - val |= BIT(g->intr_status_bit);
> - else
> - val &= ~BIT(g->intr_status_bit);
> + val = (g->intr_ack_high) ? BIT(g->intr_status_bit) : 0;
> msm_writel_intr_status(val, pctrl, g);
>
> if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> --
> 2.29.2.729.g45daf8777d-goog
>
