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;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
References: <20181219221105.3004-1-ilina@codeaurora.org> <20181219221105.3004-6-ilina@codeaurora.org> <154533621302.79149.15228907259643696166@swboyd.mtv.corp.google.com> <20190116231328.GA20369@codeaurora.org>
User-Agent: alot/0.8
Cc:     evgreen@chromium.org, marc.zyngier@arm.com,
        linux-kernel@vger.kernel.org, rplsssn@codeaurora.org,
        linux-arm-msm@vger.kernel.org, thierry.reding@gmail.com,
        bjorn.andersson@linaro.org
From:   Stephen Boyd <swboyd@chromium.org>
In-Reply-To: <20190116231328.GA20369@codeaurora.org>
Message-ID: <154827724678.136743.1337362671388538883@swboyd.mtv.corp.google.com>
To:     Lina Iyer <ilina@codeaurora.org>
Date:   Wed, 23 Jan 2019 13:00:46 -0800
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Lina Iyer (2019-01-16 15:13:28)
> On Thu, Dec 20 2018 at 13:03 -0700, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-12-19 14:11:03)
> >> +
> >> +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned =
int virq,
> >> +                                unsigned int nr_irqs, void *arg)
> >> +{
> >> +       int ret;
> >> +       irq_hw_number_t hwirq;
> >> +       struct gpio_chip *gc =3D domain->host_data;
> >> +       struct msm_pinctrl *pctrl =3D gpiochip_get_data(gc);
> >> +       struct irq_fwspec *fwspec =3D arg;
> >> +       struct qcom_irq_fwspec parent =3D { };
> >> +       unsigned int type;
> >> +
> >> +       ret =3D msm_gpio_domain_translate(domain, fwspec, &hwirq, &typ=
e);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       ret =3D irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> >> +                                           &pctrl->irq_chip, gc);
> >> +       if (ret < 0)
> >> +               return ret;
> >> +
> >> +       if (!domain->parent)
> >> +               return 0;
> >> +
> >> +       parent.fwspec.fwnode      =3D domain->parent->fwnode;
> >> +       parent.fwspec.param_count =3D 2;
> >> +       parent.fwspec.param[0]    =3D hwirq;
> >> +       parent.fwspec.param[1]    =3D type;
> >> +
> >> +       ret =3D irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &p=
arent);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       if (parent.mask)
> >> +               set_bit(hwirq, pctrl->wakeup_masked_irqs);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +/*
> >> + * TODO: Get rid of this and push it into gpiochip_to_irq()
> >
> >Hmm.. yeah we need to do this still. I think we can have a generic two
> >cell function similar to irq_domain_xlate_twocell() that does the fwspec
> >creation and uses some of the things that we pass to
> >gpiochip_irqchip_add(), like the default level type. This existing
> >function is not good to have, so there's work to do to get rid of this.
> >
>=20
> >I was also thinking that maybe we can make the alloc function above take
> >a struct gpio_irq_fwspec structure that tells the alloc function what
> >gpiochip the irq is for. That would mean that we need to change the
> >gpio_to_irq() function below to be generic and stuff the chip inside the
> >fwspec wrapper structure:
> >
> >       struct gpio_irq_fwspec {
> >               struct irq_fwspec fwspec;
> >               struct gpio_chip *chip;
> >               unsigned int offset;
> >       };
> >
> >but I seem to recall that was not working for some reason.
> >
> I was thinking about this. If I understand you correctly, we want to
> generalize the .translate and .alloc functions. We could move the
> .translate to generic however, the alloc would still need to be specific
> for the parent.mask. But we can do this without the gpio_irq_fwspec. I
> presume you suggest this structure so we could pass the hwirq and type
> to the .alloc function. but we have that in the fwspec. What am I
> missing?

I'm trying to flatten the irq number space into a single integer while
letting it cross multiple gpio chips. Similar to how gpiolib flattens
the gpio number space across multiple gpio chips. The goal being to make
gpiochip_to_irq() do this all for us without having to implement it in
each gpio driver, but maybe that isn't needed or wise if the goal is to
move drivers away from taking a gpio number and converting it into an
irq with gpio_to_irq() to begin with.

FWIW, the SPMI PMIC gpio chip conversion patches didn't fix this problem
and Brian just called irq_create_fwspec_mapping() from the to_irq()
hook. So maybe this can be cleaned up later.