Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5d71a6db.1c69fb81.1bc1c.9225@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20190829181203.2660-7-ilina@codeaurora.org>
References: <20190829181203.2660-1-ilina@codeaurora.org> <20190829181203.2660-7-ilina@codeaurora.org>
To:     Lina Iyer <ilina@codeaurora.org>, evgreen@chromium.org,
        linus.walleij@linaro.org, marc.zyngier@arm.com
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        bjorn.andersson@linaro.org, mkshah@codeaurora.org,
        linux-gpio@vger.kernel.org, rnayak@codeaurora.org,
        Lina Iyer <ilina@codeaurora.org>
Subject: Re: [PATCH RFC 06/14] drivers: irqchip: pdc: additionally set type in SPI config registers
User-Agent: alot/0.8.1
Date:   Thu, 05 Sep 2019 17:22:50 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Lina Iyer (2019-08-29 11:11:55)
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index ad1faf634bcf..bf5f98bb4d2b 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -100,6 +112,57 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
>         irq_chip_unmask_parent(d);
>  }
> =20
> +static u32 __spi_pin_read(unsigned int pin)
> +{
> +       void __iomem *cfg_reg =3D spi_cfg->base + pin * 4;
> +       u64 scm_cfg_reg =3D spi_cfg->start + pin * 4;
> +
> +       if (spi_cfg->scm_io) {
> +               unsigned int val;
> +
> +               qcom_scm_io_readl(scm_cfg_reg, &val);
> +               return val;
> +       } else {
> +               return readl(cfg_reg);
> +       }

Please remove the else and just return readl() result instead.

> +}
> +
> +static void __spi_pin_write(unsigned int pin, unsigned int val)
> +{
> +       void __iomem *cfg_reg =3D spi_cfg->base + pin * 4;
> +       u64 scm_cfg_reg =3D spi_cfg->start + pin * 4;
> +
> +       if (spi_cfg->scm_io)
> +               qcom_scm_io_writel(scm_cfg_reg, val);
> +       else
> +               writel(val, cfg_reg);
> +}
> +
> +static int spi_configure_type(irq_hw_number_t hwirq, unsigned int type)
> +{
> +       int spi =3D hwirq - 32;
> +       u32 pin =3D spi / 32;
> +       u32 mask =3D BIT(spi % 32);
> +       u32 val;
> +       unsigned long flags;
> +
> +       if (!spi_cfg)
> +               return 0;
> +
> +       if (pin * 4 > spi_cfg->size)
> +               return -EFAULT;
> +
> +       raw_spin_lock_irqsave(&pdc_lock, flags);

Ah I don't think the regmap would use a raw spinlock, so that's another
hurdle to get over here.

> +       val =3D __spi_pin_read(pin);
> +       val &=3D ~mask;
> +       if (type & IRQ_TYPE_LEVEL_MASK)
> +               val |=3D mask;
> +       __spi_pin_write(pin, val);

Does monitoring level triggered interrupts matter? I'm asking if the
whole thing can be configured to monitor for edges regardless of trigger
type and then let the level handling be done by the GIC after the wakeup
or when the device is active.

> +       raw_spin_unlock_irqrestore(&pdc_lock, flags);
> +
> +       return 0;
> +}
> +
>  /*
>   * GIC does not handle falling edge or active low. To allow falling edge=
 and
>   * active low interrupts to be handled at GIC, PDC has an inverter that =
inverts